Skip to content

Conversation

@yoonghm
Copy link
Contributor

@yoonghm yoonghm commented Sep 11, 2020

Change val to unsigned char before being tested.

Change `val` to `unsigned char` before being tested.
@oranagra oranagra changed the title Fix warning in redis/issues/7784 Fix compilation warning in jemalloc's malloc_vsnprintf Sep 13, 2020
@oranagra
Copy link
Member

@yoonghm i don't see this warning on my Ubuntu.
which OS/compiler are you using?

i'm concerned about merge conflicts when we upgrade jemalloc, or what's worse, if we don't get merge conflicts, but get bugs instead.

I see the len argument to that macro is already unsigned char in most cases, maybe with the exception of this call:

GET_ARG_NUMERIC(val, 'p');

maybe it is enough to add casting in the above line instead? i.e. (unsigned char)'p'?
this is more local and thus less likely to cause an side effects if the code is changed.

@yoonghm
Copy link
Contributor Author

yoonghm commented Sep 21, 2020

The problem has been fixed in https://github.com/jemalloc/jemalloc/blob/dev/src/malloc_io.c#L367
image

@oranagra
Copy link
Member

ok, so you say this change reflects a change done in a future version of jemalloc.
i can live with that. i'll update the commit comment when squashing.

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Sep 21, 2020
@oranagra oranagra merged commit 9216b96 into redis:unstable Sep 21, 2020
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 4, 2020
Change `val` to `unsigned char` before being tested.
The fix is identical to the one that's been made in upstream jemalloc.

warning is:
src/malloc_io.c: In function ‘malloc_vsnprintf’:
src/malloc_io.c:369:2: warning: case label value exceeds maximum value for type
  369 |  case '?' | 0x80:      \
      |  ^~~~
src/malloc_io.c:581:5: note: in expansion of macro ‘GET_ARG_NUMERIC’
  581 |     GET_ARG_NUMERIC(val, 'p');
      |     ^~~~~~~~~~~~~~~
oranagra added a commit that referenced this pull request Oct 18, 2021
Upgraded to jemalloc 5.2.1 from 5.1.0.
Cherry picked all relevant fixes (by diffing our 5.1.0 to upstream 5.10 and finding relevant commits).
Details of what was done:

[cherry-picked] fd7d51c 2021-05-03 Resolve nonsense static analysis warnings (Oran Agra)
[cherry-picked] 448c435 2020-09-29 Fix compilation warnings in Lua and jemalloc dependencies (#7785) (YoongHM)
[skipped - already in upstream] 9216b96 2020-09-21 Fix compilation warning in jemalloc's malloc_vsnprintf (#7789) (YoongHM)
[cherry-picked] 88d71f4 2020-05-20 fix a rare active defrag edge case bug leading to stagnation (Oran Agra)
[skipped - already in upstream] 2fec7d9 2019-05-30 Jemalloc: Avoid blocking on background thread lock for stats.
[cherry-picked] 920158e 2018-07-11 Active defrag fixes for 32bit builds (again) (Oran Agra)
[cherry-picked] e8099ca 2018-06-26 add defrag hint support into jemalloc 5 (Oran Agra)
[re-done] 4e729fc 2018-05-24 Generate configure for Jemalloc. (antirez)

Additionally had to do this:
7727cc2 2021-10-10 Fix defrag to support sharded bins in arena (added in v5.2.1) (Yoav Steinberg)

When reviewing please look at all except the first commit which is just replacing 5.1.0 with 5.2.1 sources.
Also I think we should merge this without squashing to preserve the changes we did to to jemalloc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants