Clean compilation with -Wextra by gnzlbg · Pull Request #1200 · jemalloc/jemalloc

gnzlbg · 2018-05-03T13:00:40Z

Clean compilation -Wextra

Before this pull-request jemalloc produced many warnings when compiled with -Wextra
with both Clang and GCC. This pull-request fixes the issues raised by these warnings
or suppresses them if they were spurious at least for the Clang and GCC
versions covered by CI.

This pull-request:

adds JEMALLOC_DIAGNOSTIC macros: JEMALLOC_DIAGNOSTIC_{PUSH,POP} are
used to modify the stack of enabled diagnostics. The
JEMALLOC_DIAGNOSTIC_IGNORE_... macros are used to ignore a concrete
diagnostic.
adds JEMALLOC_FALLTHROUGH macro to explicitly state that falling
through case labels in a switch statement is intended
locally supresses many unused argument warnings by adding missing
UNUSED annotations
locally suppresses some -Wextra diagnostics:
- -Wmissing-field-initializer is buggy in older Clang and GCC versions, not
  understanding that, in C, = {0} is a common idiom to initialize a struct to zero
- -Wtype-bounds is suppressed in a particular situation where a generic
  macro, used in multiple different places, compares an unsigned integer for
  smaller than zero, which is always true.
- -Walloc-larger-than-size= diagnostics warn when an allocation function is
  called with a size that is too large (out-of-range). These are suppressed in
  the parts of the tests where jemalloc explicitly does this to test that the
  allocation functions fail properly.
fixes a bug in the log.c tests where an array was being written out-of bounds,
which was probably invoking undefined behavior.

Closes #1196 .

davidtgoldblatt · 2018-05-03T19:21:52Z

include/jemalloc/internal/tcache_types.h


 /* Used in TSD static initializer only. Real init in tcache_data_init(). */
-#define TCACHE_ZERO_INITIALIZER {0}
+#define TCACHE_ZERO_INITIALIZER                      \


This one is a bridge too far IMO; it might be better to disable the warning on OS X at this point.

This warning triggers on Linux with clang and some GCC versions as well (gcc 8.1 doesn't trigger the warning, but IIRC GCC 4.8 does).

I am not 100% familiar with C's initialization rules (they are different from the C++ ones). AFAIK = {0}; in C should initialize the memory of the whole object to zero, independently of how many fields the object has, and that's the correct way to do that.

I think I would prefer to add two macros to the "jemalloc internal macros" header, to:

JEMALLOC_POP_WARNING_MISSING_FIELD_INITIALIZERS: disable the warning

JEMALLOC_PUSH_WARNING_MISSING_FIELD_INITIALIZERS: re-enable the warning

And manually use those in the places (fine-grained) or the whole file where the warning actually triggers.

I am not a fan of disabling the warning in CI and call it a day, because people compiling with older compilers will still get it.

What do you think about this alternatives?

davidtgoldblatt · 2018-05-03T19:22:40Z

Thanks! As a general nitty thing, could you rebase/edit this to match the jemalloc style (both for the code + commit messages)?

In particular:

Multi-line comments look like this:

/*
 * Line 1
 * Line 2
 */

C-style comments only (no //).
Comments are capitalized sentences
Commit message title is capitalized, and describes what change the commit makes (and the body describes why, if it's not obvious). E.g. instead of "apple clang 9.0 compiles with -Wextra"
Instead of fixup commits, rebase the two together so that the linear history looks clean.

gnzlbg · 2018-05-04T08:01:27Z

@davidtgoldblatt

As a general nitty thing, could you rebase/edit this to match the jemalloc style (both for the code + commit messages)?

Sure can do. Does jemalloc use clang-format or similar for formatting / linting on this ?

Instead of fixup commits,

I see, I thought this "commit stream" would get squashed on merge into a single commit with the body and title of the PR message, which I've kept up to date. I can keep the commit messages separate if you prefer, but they won't be really meaningful (e.g. UNUSED is not fixed in a single commit, but in a bunch of them as my laptop, workstation, and then CI all triggered these in different places).

gnzlbg · 2018-05-09T08:54:38Z

@interwq could you also review this?

gnzlbg · 2018-05-09T09:06:22Z

I've rebased.

interwq

Thanks for working on this @gnzlbg! Mostly just nitpicks.

interwq · 2018-05-09T18:11:02Z

include/jemalloc/internal/atomic_gcc_sync.h

 ATOMIC_INLINE type							\
 atomic_exchange_##short_type(atomic_##short_type##_t *a, type val,	\
-    atomic_memory_order_t mo) {						\
+    UNUSED atomic_memory_order_t mo) {						\


nit: align the \ with other lines. Probably will need to split the line at L132 and L145.

I think I've done this, but this line doesn't look at all like this to me on my editor, is this using tabs or spaces ?

FWIW this is how all this code looks like to me:

It appears that tabs are being used to indent the \. There is probably a single number of spaces that a tab must expand to for this to look properly aligned, but for any other number of spaces, this looks extremely chaotic.

I could align all this code just using spaces easily, but I don't know if this is what you want.

FYI I am using emacs, so maybe there is a way to make it behave properly for jemalloc, but I don't really know how to set up a config explicitly for a single directory, nor what that config should look like. AFAICT I am using the default emacs config which just only uses spaces, never tabs, and my tab key just calls clang format in your project, which reformats the whole project to something different, so I can't really use it :/

interwq · 2018-05-09T18:14:43Z

include/jemalloc/internal/jemalloc_internal_macros.h

+  && defined(JEMALLOC_HAVE_ATTR) && (__GNUC__ >= 7)
+#define JEMALLOC_FALLTHROUGH JEMALLOC_ATTR(fallthrough);
+#else
+#define JEMALLOC_FALLTHROUGH


To be safe, define it as /* falls through */ here?

What does that protect us from exactly?

I could do something like do { } while(0) if you want to require a ; after JEMALLOC_FALLTHROUGH.

If we have an unknown set up, having the old comment style may help avoid warnings in that case.

Makes sense, I'll add that.

interwq · 2018-05-09T18:23:50Z

include/jemalloc/internal/mutex.h

 #  define MALLOC_MUTEX_INITIALIZER
 #elif (defined(JEMALLOC_OS_UNFAIR_LOCK))
-#  define MALLOC_MUTEX_INITIALIZER					\
+#  if defined(JEMALLOC_DEBUG)


Branching on JEMALLOC_DEBUG for the warning feels a bit too much to me. I'd be willing to even disable the warning, if it cannot be workaround.

I think the "problem" here is that the layout of the types changes depending on whether JEMALLOC_DEBUG is enabled or not. The whole = {0} thing triggering a warning looks spurious to me, so maybe we should just disable that warning globally.

interwq · 2018-05-09T18:24:12Z

include/jemalloc/internal/mutex.h

 #  define MALLOC_MUTEX_INITIALIZER					\
     {{{LOCK_PROF_DATA_INITIALIZER, 0}},				\
-      WITNESS_INITIALIZER("mutex", WITNESS_RANK_OMIT)}
+         WITNESS_INITIALIZER("mutex", WITNESS_RANK_OMIT)}


nit: unnecessary space change.

interwq · 2018-05-09T18:31:19Z

src/jemalloc.c


+/*
+ * One of the CONF_MIN macros below expands, in one of the use points,
+ * to "usigned integer < 0", which is always false, triggering the


nit: typo usigned.

Also this disables it for a pretty wide range. Maybe try limiting the scope if possible?

I couldn't reproduce this locally but it happened on CI so it was hard for me to nail down exactly which call to CONF_MIN was causing this. I will try to move this to the single call where this happens.

Due to the way the code below is structured, its really hard to tell where the actual CONF_#..._#... macro that expands to the code with the issue even is, and in which instance of the macro invokation it happens :/

I've limited it a bit more, and will send a couple of commits limiting it a bit further, but since I cannot reproduce this locally its going to take a while. Expect the PR to be in a semi-broken state until then.

interwq · 2018-05-09T18:32:13Z

src/rtree.c


 static void
-rtree_node_dalloc_impl(tsdn_t *tsdn, rtree_t *rtree, rtree_node_elm_t *node) {
+rtree_node_dalloc_impl(UNUSED tsdn_t *tsdn, UNUSED rtree_t *rtree, UNUSED rtree_node_elm_t *node) {


nit: line length > 80

I'm not at all a fan of warnings about unused function parameters, and I'd rather we configure to disable such warnings when necessary, rather than bulking up our function declarations with mitigations.

@jasone given that a lot of functions already do this, I think that disabling these globally might be a valid option.

When updating these I see that many functions take parameters that they just never use. I can only guess why, but if the reasons for these are valid, disabling the warning globally for jemalloc is the way to go.

In many cases it's indeed intentional, to keep similar functions have the same set of input.

That's what it felt like. So maybe disabling this globally and removing all the UNUSED annotations is the way to go. It seems that a lot of code would require these annotations otherwise, and the wins are small: adding them did not find any bugs and it was painful to do so.

So I've removed all usages of UNUSED function parameters. I've left unused for unused functions and unused variables, but those appear only 4 times in the code.

I've disabled the "unused function parameter" warning globally for each translation unit that includes the jemalloc_internal_macros.h header. It is never re-enabled since users cannot include this header anyways.

interwq · 2018-05-09T19:34:45Z

test/unit/log.c


 	count = 0;
-	strcpy(log_var_names, "l1.a");
+  log_var_names[0] =  'l';


This doesn't feel right. Should we increase the size of log_var_names instead?

I think this is spurious, since we don't actually run any of this code when logging is disabled.

The problem is that in some situations in which this code is compiled JEMALLOC_LOG is not defined, so JEMALLOC_LOG_VAR_BUFSIZE == 1, and the strcpy invokes undefined behavior.

But in those cases, config_log is false, and so the test_skip_if(!config_log); above returns early, so that this code doesn't execute. (Or, if not, then that's a true bug that we should fix in the implementation).

@davidtgoldblatt maybe these tests should then just be #ifdefed out if JEMALLOC_LOG is not defined?

@davidtgoldblatt an alternative is to leave it as it was and just silence the warning

davidtgoldblatt · 2018-05-09T19:52:53Z

I see, I thought this "commit stream" would get squashed on merge into a single commit with the body and title of the PR message, which I've kept up to date.

We actually disable every github merge option except "rebase and merge", which feels like the safest way to avoid polluting the commit history without accidentally squashing commits (sometimes we'll have big pull requests where we truly want the linear history preserved). I think this is different than the way a lot of projects do things.

gnzlbg · 2018-05-09T22:40:16Z

We actually disable every github merge option except "rebase and merge"

That is sensible. I'd like to nail the "code changes" first, and I can do whatever is best for reviewers, be that "rebase at the end" so that they only have to look at the commits after the last one they checked out previously, or continuously rebase, so that the history is always the one that will be merged.

gnzlbg · 2018-05-11T12:02:38Z

I still need to review if the latest changes use jemalloc formatting but that will take a while since I have to do this manually.

I also still don't know what to do about the log.c issue.

gnzlbg · 2018-05-17T12:36:15Z

ping: this now builds correctly. I am still unsure about what to do over the following things

log.c tests. I could: #ifdef out if JEMALLOC_LOG is not defined, or I could also suppress the warning if JEMALLOC_LOG is not defined for that file. Thoughts?
branching on JEMALLOC_DEBUG in some places (2 or 3 places) to suppress warnings (mentioned by @interwq ): I am not sure whether I'd prefer to leave that as is, but I don't think we should always suppress the warning either. Maybe I can just suppress the warning globally if JEMALLOC_DEBUG is defined but leave it on otherwise?

What do you guys think?

davidtgoldblatt · 2018-05-18T17:32:41Z

The issue with #define-ing away stuff that triggers warnings is that it has historically led to situations where changes are very brittle (things appear to compile OK when writing code, but break in silly ways when changing configure settings or platforms while testing), so we've made an effect to move away from that sort of setup.

For stuff like this, if the only ways to make things clean w.r.t. a warning are to obfuscate the code to hide it from a warning or #define it out of executing, I think better would be to just disable the warning. Like, I only think we should be turning on warnings to the extent that it's a dialect of C we want to restrict ourselves to (for safety, readability, whatever). Being -Wextra clean for its own sake doesn't buy us a lot if we can't turn that into day-to-day benefits.

Which compiler versions are you using to get these settings? If we're putting in the work to stay clean with respect to a particular warning, we should at least try to prevent regressions over time.

gnzlbg · 2018-05-18T22:05:32Z

I think better would be to just disable the warning.

@davidtgoldblatt so I think I will just disable the warning for that file if JEMALLOC_LOG is not defined and call it a day. It does not add anything valuable in this particular case.

Which compiler versions are you using to get these settings?

I'm using latest apple clang, clang trunk, gcc 8.1, and well all the compilers used on CI.

If we're putting in the work to stay clean with respect to a particular warning, we should at least try to prevent regressions over time.

We can add a travis ci buildbot that requires osx_image: xcode9.2 to test the latest apple clang (or maybe just upgrade the current apple build bots to use the latest xcode version by setting their osx_image key appropriately), and maybe add one image that maybe tests gcc 8.1, but I wouldn't add a whole bunch of build bots just to test for this. In any case, testing with the latest released compiler versions is something that makes sense doing because it is what most distros upgrading jemalloc are going to use anyways.

gnzlbg · 2018-05-22T15:22:11Z

So I've reverted the log.c changes, I think I've addressed everything.

gnzlbg · 2018-06-04T20:58:34Z

Ping.

davidtgoldblatt · 2018-06-05T11:47:28Z

This looks reasonable as a whole. A couple nits:

C-style comments only, please
Let's squash / rebase in the log commit as appropriate

After that, I think all that's left is to make sure nothing breaks on the handful of environments we care about but don't test. Apologies for the delay, this has been a somewhat frenzied period (an intern arriving and the C++ committee meeting).

gnzlbg · 2018-06-15T08:35:21Z

@davidtgoldblatt I've fixed the C++ comment and squashed/rebased into a meaningful commit message.

gnzlbg · 2018-06-25T09:00:45Z

ping

gnzlbg · 2018-07-05T08:25:02Z

ping

davidtgoldblatt · 2018-07-05T08:33:09Z

First thing on my plate tomorrow morning. Sorry for the long delay.

davidtgoldblatt · 2018-07-05T18:23:20Z

Looks like the log test still has some fixes needed (breaks with logging enabled). Is it possible to locally disable those warnings?

davidtgoldblatt · 2018-07-05T18:24:39Z

(More specific repro):

autoconf
./configure --enable-debug --enable-log
 make -j test/unit/log
test/unit/log

gnzlbg · 2018-07-05T18:25:26Z

Looks like the log test still has some fixes needed (breaks with logging enabled).

Uh, is that covered in CI? If not I should enable it as well. I will repro tomorrow morning and fix this. I can also add a script to the travis-CI that tests this particular case.

davidtgoldblatt · 2018-07-05T18:47:55Z

It's not, unfortunately. We don't really have the CI capacity to run all the configurations we care about[1] if we want quick CI turnaround (especially for dev-only features, like logging). For day-to-day stuff, I configure with ./configure --enable-debug --disable-cache-oblivious --enable-stats --enable-log --enable-prof, and that catches most issues. (run_tests.sh is more exhaustive, although not even that tests logging. I do think you're right that we should have at least something that runs with logging enabled though; we shouldn't have CI be blind to a whole unit test.

[1] Often for somewhat goofy reasons; e.g. there's no reason logging couldn't be a runtime option, avoiding us having to re-configure and rebuild to test it. It was just more convenient to make it configure-time, though.

gnzlbg · 2018-07-06T10:50:51Z

So i've added a new builder to travis testing the configuration you mentioned, and also running the log unit test in a commit, and fixed it in the next commit.

I've reproduced the failure locally, but the commit that added the test without the fix did not fail on travis: https://travis-ci.org/jemalloc/jemalloc/jobs/400820090 :/ So I'd suspect that something with ci is still off.

gnzlbg · 2018-07-06T10:56:09Z

Duh: https://travis-ci.org/jemalloc/jemalloc/jobs/400820090#L105

It appears that that travis fetches the head of the branch of a PR on every job, meaning, if you push two commits that run the tests, CI won't run for the independent commits, but only for the tip of the PR branch (that is, by the time the buildbot running the log test without the fix was started, the fix was already commit, so it got pulled, and that's why it passed).

davidtgoldblatt · 2018-07-09T21:28:10Z

Looks good. If you could squash/rebase as appropriate (and clean up the commit messages some), I'll merge.

davidtgoldblatt · 2018-07-09T21:29:15Z

(Preferably, first commit being adding the new travis config, and second commit being the -Wextra change)

Before this commit jemalloc produced many warnings when compiled with -Wextra with both Clang and GCC. This commit fixes the issues raised by these warnings or suppresses them if they were spurious at least for the Clang and GCC versions covered by CI. This commit: * adds `JEMALLOC_DIAGNOSTIC` macros: `JEMALLOC_DIAGNOSTIC_{PUSH,POP}` are used to modify the stack of enabled diagnostics. The `JEMALLOC_DIAGNOSTIC_IGNORE_...` macros are used to ignore a concrete diagnostic. * adds `JEMALLOC_FALLTHROUGH` macro to explicitly state that falling through `case` labels in a `switch` statement is intended * Removes all UNUSED annotations on function parameters. The warning -Wunused-parameter is now disabled globally in `jemalloc_internal_macros.h` for all translation units that include that header. It is never re-enabled since that header cannot be included by users. * locally suppresses some -Wextra diagnostics: * `-Wmissing-field-initializer` is buggy in older Clang and GCC versions, where it does not understanding that, in C, `= {0}` is a common C idiom to initialize a struct to zero * `-Wtype-bounds` is suppressed in a particular situation where a generic macro, used in multiple different places, compares an unsigned integer for smaller than zero, which is always true. * `-Walloc-larger-than-size=` diagnostics warn when an allocation function is called with a size that is too large (out-of-range). These are suppressed in the parts of the tests where `jemalloc` explicitly does this to test that the allocation functions fail properly. * adds a new CI build bot that runs the log unit test on CI. Closes jemalloc#1196 .

gnzlbg · 2018-07-09T21:32:11Z

oops, I saw your second comment after pushing (I squashed everything onto the same commit)

davidtgoldblatt · 2018-07-10T04:41:21Z

Thanks for the diff! (And, thanks for pushing through on the most-commented pull request we have so far).

davidtgoldblatt reviewed May 3, 2018

View reviewed changes

gnzlbg force-pushed the wextra branch from aadee20 to 502524c Compare May 9, 2018 09:05

interwq reviewed May 9, 2018

View reviewed changes

gnzlbg force-pushed the wextra branch from fff5f7e to 923e30e Compare May 11, 2018 10:21

gnzlbg force-pushed the wextra branch from 2bd04ce to 9bc92f2 Compare May 11, 2018 14:24

gnzlbg force-pushed the wextra branch from 316636f to 247170e Compare May 22, 2018 14:33

gnzlbg force-pushed the wextra branch from 247170e to 9625c0c Compare June 15, 2018 08:34

gnzlbg force-pushed the wextra branch from ab5b8ee to 9c5afa8 Compare July 9, 2018 21:31

davidtgoldblatt merged commit 3d29d11 into jemalloc:dev Jul 10, 2018

Conversation

gnzlbg commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtgoldblatt commented May 3, 2018

Uh oh!

gnzlbg commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnzlbg commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnzlbg commented May 9, 2018

Uh oh!

interwq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnzlbg May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnzlbg May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtgoldblatt commented May 9, 2018

gnzlbg commented May 3, 2018 •

edited

Loading

gnzlbg commented May 4, 2018 •

edited

Loading

gnzlbg commented May 9, 2018 •

edited

Loading

gnzlbg May 9, 2018 •

edited

Loading

gnzlbg May 9, 2018 •

edited

Loading