Silence all warnings when building with GCC on Linux #44350

giordano · 2022-02-25T23:56:52Z

With this PR I can do a clean build from scratch (at least after git clean -fxd) with make CFLAGS=-Werror CXXFLAGS=-Werror when using GCC 11.1.0 on Linux. Clang is way more picky.

@staticfloat @vtjnash does it make sense to enable -Werror in CI at least for the platforms where we use GCC? Now that there is a (partially) clean state it'd be good to not introduce new warnings.

giordano · 2022-02-26T01:45:33Z

In https://build.julialang.org/#/builders/41/builds/3390/steps/7/logs/stdio I see another warning which I didn't get locally

In file included from /buildworker/worker/package_linux64/build/src/codegen.cpp:1673:0:
/buildworker/worker/package_linux64/build/src/cgutils.cpp: In function 'jl_cgval_t emit_setfield(jl_codectx_t&, jl_datatype_t*, const jl_cgval_t&, size_t, jl_cgval_t, jl_cgval_t, bool, llvm::AtomicOrdering, llvm::AtomicOrdering, bool, bool, bool, bool, bool, const jl_cgval_t*, const string&)':
/buildworker/worker/package_linux64/build/src/cgutils.cpp:3339:37: warning: 'ModifyBB' may be used uninitialized in this function [-Wmaybe-uninitialized]
             ctx.builder.CreateCondBr(Success, XchgBB, ismodifyfield ? ModifyBB : DoneBB);
             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

ModifyBB is set at

julia/src/cgutils.cpp

Lines 3302 to 3306 in a2e4226

    
           if (ismodifyfield) { 
        
               ModifyBB = BasicBlock::Create(ctx.builder.getContext(), "modify_xchg", ctx.f); 
        
               ctx.builder.CreateBr(ModifyBB); 
        
               ctx.builder.SetInsertPoint(ModifyBB); 
        
           }

and then used at

julia/src/cgutils.cpp

Line 3339 in a2e4226

ctx.builder.CreateCondBr(Success, XchgBB, ismodifyfield ? ModifyBB : DoneBB);

only if ismodifyfield is true, which is the same condition when it's set. I wonder if newer GCC got smarter?

Edit: warning fixed in the following commit.

giordano · 2022-02-26T02:21:49Z

Instead on aarch64 we get plenty of warnings like

In file included from /buildworker/worker/package_linuxaarch64/build/usr/include/libunwind.h:7:0,
                 from /buildworker/worker/package_linuxaarch64/build/src/julia_internal.h:1019,
                 from /buildworker/worker/package_linuxaarch64/build/src/jloptions.c:7:
/buildworker/worker/package_linuxaarch64/build/usr/include/libunwind-aarch64.h:60:9: warning: empty struct has size 0 in C, size 1 in C++ [-Wc++-compat]
 typedef struct
         ^~~~~~
/buildworker/worker/package_linuxaarch64/build/usr/include/libunwind-aarch64.h:169:16: warning: empty struct has size 0 in C, size 1 in C++ [-Wc++-compat]
 typedef struct unw_tdep_save_loc
                ^~~~~~~~~~~~~~~~~

which come from libunwind... I guess at the moment we can do little about them apart from disabling the corresponding warnings

vtjnash · 2022-02-26T02:48:49Z

libunwind is usually pretty good about taking patches. Looks like some platforms put char unused; there, but not aarch64 or x86 and probably others.

giordano · 2022-02-26T17:55:46Z

Summary of the compiler warnings:

I don't see any for linux64 🥳

for linux32 there are

/buildworker/worker/package_linux32/build/src/jitlayers.cpp: In member function 'virtual CompilerResultT JuliaOJIT::CompilerT::operator()(llvm::Module&)':
/buildworker/worker/package_linux32/build/src/jitlayers.cpp:485:94: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'int' [-Wformat=]
             jl_printf(dump_llvm_opt_stream, "        basicblocks: %lu\n", countBasicBlocks(F));
                                                                           ~~~~~~~~~~~~~~~~~~~^
/buildworker/worker/package_linux32/build/src/jitlayers.cpp:551:94: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'int' [-Wformat=]
             jl_printf(dump_llvm_opt_stream, "        basicblocks: %lu\n", countBasicBlocks(F));
                                                                           ~~~~~~~~~~~~~~~~~~~^

[...]

In file included from /buildworker/worker/package_linux32/build/src/codegen.cpp:2227:0:
/buildworker/worker/package_linux32/build/src/intrinsics.cpp: In function 'jl_cgval_t generic_cast(jl_codectx_t&, JL_I::intrinsic, llvm::Instruction::CastOps, const jl_cgval_t*, bool, bool)':
/buildworker/worker/package_linux32/build/src/intrinsics.cpp:558:98: warning: 'llvm::LoadInst* llvm::IRBuilderBase::CreateLoad(llvm::Value*, bool, const llvm::Twine&)' is deprecated: Use the version that explicitly specifies the loaded type instead [-Wdeprecated-declarations]
         from  = ctx.builder.CreateLoad(jlfloattemp_var, /*force this to load from the stack*/true);
                                                                                                  ^
In file included from /buildworker/worker/package_linux32/build/usr/include/llvm/ADT/APInt.h:18:0,
                 from /buildworker/worker/package_linux32/build/usr/include/llvm/CodeGen/TargetSubtargetInfo.h:16,
                 from /buildworker/worker/package_linux32/build/src/codegen.cpp:32:
/buildworker/worker/package_linux32/build/usr/include/llvm/IR/IRBuilder.h:1686:39: note: declared here
   LLVM_ATTRIBUTE_DEPRECATED(LoadInst *CreateLoad(Value *Ptr,
                                       ^
/buildworker/worker/package_linux32/build/usr/include/llvm/Support/Compiler.h:320:74: note: in definition of macro 'LLVM_ATTRIBUTE_DEPRECATED'
 #define LLVM_ATTRIBUTE_DEPRECATED(decl, message) [[deprecated(message)]] decl
                                                                          ^~~~
/buildworker/worker/package_linux32/build/src/codegen.cpp: In function 'bool emit_f_opfield(jl_codectx_t&, jl_cgval_t*, jl_value_t*, const jl_cgval_t*, size_t, const jl_cgval_t*)':
/buildworker/worker/package_linux32/build/src/codegen.cpp:2717:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
             if (i > 0 && i <= jl_datatype_nfields(uty))

and the libunwind warnings mentioned above

linuxaarch64 only libunwind warnings

freebsd and macos64 have plenty of

./julia.h:397:9: warning: anonymous types declared in an anonymous union are an extension [-Wnested-anon-types]
        struct {
        ^
./julia.h:406:9: warning: anonymous types declared in an anonymous union are an extension [-Wnested-anon-types]
        struct {
        ^

which is tracked by Issues related to the purity modeling PR #44314

win32 and win64 have many warnings like

/cygdrive/c/buildbot/worker/package_win64/build/src/support/utf8.c: In function 'u8_escape_wchar':
/cygdrive/c/buildbot/worker/package_win64/build/src/support/utf8.c:401:35: warning: unknown conversion type character 'h' in format [-Wformat=]
  401 |         return snprintf(buf, sz, "\\x%.2hhx", (unsigned char)ch);
      |                                   ^~~~~~~~~~~
/cygdrive/c/buildbot/worker/package_win64/build/src/support/utf8.c:401:35: warning: too many arguments for format [-Wformat-extra-args]

[...]

/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp: In function 'jl_value_t* (* _jl_compile_codeinst(jl_code_instance_t*, jl_code_info_t*, size_t))(jl_value_t*, jl_value_t**, uint32_t, _jl_code_instance_t*)':
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp:195:46: warning: unknown conversion type character 'l' in format [-Wformat=]
  195 |             jl_printf(dump_compiles_stream, "%" PRIu64 "\t\"", end_time - start_time);
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp:195:46: warning: too many arguments for format [-Wformat-extra-args]
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp: In member function 'virtual CompilerResultT JuliaOJIT::CompilerT::operator()(llvm::Module&)':
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp:485:46: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long int' [-Wformat=]
  485 |             jl_printf(dump_llvm_opt_stream, "        basicblocks: %lu\n", countBasicBlocks(F));
      |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~~~~
      |                                                                                            |
      |                                                                                            long long int
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp:540:42: warning: unknown conversion type character 'l' in format [-Wformat=]
  540 |         jl_printf(dump_llvm_opt_stream, "  time_ns: %" PRIu64 "\n", end_time - start_time);
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp:540:42: warning: too many arguments for format [-Wformat-extra-args]
/cygdrive/c/buildbot/worker/package_win64/build/src/jitlayers.cpp:551:46: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long int' [-Wformat=]
  551 |             jl_printf(dump_llvm_opt_stream, "        basicblocks: %lu\n", countBasicBlocks(F));
      |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~~~~
      |                                                                                            |
      |                                                                                            long long int

Again, what about enabling -Werror in CI at least for linux64 now that it's clean?

DilumAluthge · 2022-02-26T18:05:57Z

Again, what about enabling -Werror in CI at least for linux64 now that it's clean?

This makes sense to me! And then we can add other platforms one by one as we fix then.

@staticfloat @vtjnash Are you cool with this idea? If so, I'll give @giordano directions to the two places that he'll need to implement this change.

giordano · 2022-03-07T16:39:31Z

Bump 🙂

staticfloat · 2022-03-08T19:28:56Z

Yeah, I'm cool with this. I say merge this now, and then add -Werror after we transition to buildkite within the next day or two.

giordano · 2022-03-10T22:01:40Z

Does anyone want to review the changes? I think they should all be mostly uncontroversial, the only one I'm not entirely sure is #warning -> #pragma message, in case there are other better options.

giordano · 2022-03-10T22:11:04Z

src/gc-debug.c

-        jl_safe_printf("%ld Major collection: estimate freed = %ld
-                       live = %ldm new interval = %ldm time = %ldms\n",
+        jl_safe_printf("%ld Major collection: estimate freed = %ld\n"
+                       "live = %ldm new interval = %ldm time = %ldms\n",
                       end - start, freed, live/1024/1024,
                       interval/1024/1024, pause/1000000 );
    else
-        jl_safe_printf("%ld Minor collection: estimate freed = %ld live = %ldm
-                       new interval = %ldm time = %ldms\n",
+        jl_safe_printf("%ld Minor collection: estimate freed = %ld live = %ldm\n"
+                       "new interval = %ldm time = %ldms\n",


I'm not even sure how this compiled, but I presume this was the intention of these printfs?

I am not sure it intended to have newlines

@chflood how where these lines intended to be printed? A standard printf wouldn't even compile with the previous syntax. I'm happy to fix it according to the original intention.

imciner2 · 2022-03-11T12:54:00Z

Yeah, I'm cool with this. I say merge this now, and then add -Werror after we transition to buildkite within the next day or two.

Can this be made conditional for use on the CI systems only? Adding -Werror makes packaging in linux distros that either inject their own compiler warnings or update compilers more often more complicated because usually with a new compiler comes a new warning or two that are enabled by default (or a change in the way warning semantics are used).

giordano · 2022-03-11T12:55:43Z

Can this be made conditional for use on the CI systems only?

I don't think anyone suggested to make -Werror default for everybody, which I agree would be disruptive. The proposal here is to only use it in CI.

imciner2 · 2022-03-11T12:59:07Z

I don't think anyone suggested to make -Werror default for everybody, which I agree would be disruptive. The proposal here is to only use it in CI.

Ok, that's good. I had started reading from where GitHub told me the new messages were and had forgotten the prior mentions of it in the thread since it had been so long 😆.

The former causes a compiler warning with Clang about it being a language extension, which then causes an error if building with `-Werror`.

giordano · 2022-03-24T00:04:47Z

Besides the warnings fixed by #44669 (introduced #44228), there are now new warnings introduced by #44634 (addressed by #44739). This is a whack-a-mole game

chflood · 2022-03-31T20:04:02Z

The problem with the original code was that it didn't print the right numbers on 32 bit Julia. The problem with the current code is that it gets a warning on 64 bit Julia. I will take a look. On Thu, Mar 31, 2022 at 2:25 PM Christine Flood < ***@***.***> wrote:

…

This runs when you uncomment #define GC_TIME in src/options.h On Thu, Mar 31, 2022 at 2:22 PM Mosè Giordano ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/gc-debug.c > <#44350 (comment)>: > > > - jl_safe_printf("%ld Major collection: estimate freed = %ld > - live = %ldm new interval = %ldm time = %ldms\n", > + jl_safe_printf("%ld Major collection: estimate freed = %ld\n" > + "live = %ldm new interval = %ldm time = %ldms\n", > end - start, freed, live/1024/1024, > interval/1024/1024, pause/1000000 ); > else > - jl_safe_printf("%ld Minor collection: estimate freed = %ld live = %ldm > - new interval = %ldm time = %ldms\n", > + jl_safe_printf("%ld Minor collection: estimate freed = %ld live = %ldm\n" > + "new interval = %ldm time = %ldms\n", > > @chflood <https://github.com/chflood> how where these lines intended to > be printed? A standard printf wouldn't even compile with the previous > syntax. I'm happy to fix it according to the original intention. > > — > Reply to this email directly, view it on GitHub > <#44350 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AE3CLBFIUH7RPK2TBBPFU5LVCXUPJANCNFSM5PLSGJYA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

* Silence all warnings when building with GCC on Linux * Replace `#warning` with `#pragma message` The former causes a compiler warning with Clang about it being a language extension, which then causes an error if building with `-Werror`. * Silence warning about unused variable when doing a non-debug build * Initialise another variable which can be detected as uninitialised * Fix more warnings introduced by recent changes

giordano added the building Build system, or building Julia or its dependencies label Feb 26, 2022

DilumAluthge requested review from vtjnash and staticfloat February 26, 2022 16:39

DilumAluthge requested review from staticfloat and removed request for staticfloat March 10, 2022 22:07

giordano force-pushed the mg/werror branch from 525507d to a4b2fcf Compare March 10, 2022 22:08

oscardssmith requested a review from JeffBezanson March 10, 2022 22:08

giordano commented Mar 10, 2022

View reviewed changes

giordano added 5 commits March 12, 2022 23:15

Silence all warnings when building with GCC on Linux

054fc7d

Replace #warning with #pragma message

579fdad

The former causes a compiler warning with Clang about it being a language extension, which then causes an error if building with `-Werror`.

Silence warning about unused variable when doing a non-debug build

44d16ce

Initialise another variable which can be detected as uninitialised

550ca5f

Fix more warnings introduced by recent changes

e3d8a33

giordano force-pushed the mg/werror branch 2 times, most recently from 58f05e3 to e3d8a33 Compare March 13, 2022 15:51

giordano mentioned this pull request Mar 13, 2022

Do not mark as C extern functions with C++ types #44601

Closed

imciner2 mentioned this pull request Mar 18, 2022

Remove trailing \ from last line of exported function macros #44669

Merged

oscardssmith added the triage This should be discussed on a triage call label Mar 31, 2022

staticfloat merged commit 3c08483 into JuliaLang:master Mar 31, 2022

giordano deleted the mg/werror branch March 31, 2022 17:54

giordano removed the triage This should be discussed on a triage call label Mar 31, 2022

giordano mentioned this pull request Mar 31, 2022

Mark as inline functions implemented in src/serialize.h #44739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Silence all warnings when building with GCC on Linux #44350

Silence all warnings when building with GCC on Linux #44350

giordano commented Feb 25, 2022 •

edited

Loading

giordano commented Feb 26, 2022 •

edited

Loading

giordano commented Feb 26, 2022

vtjnash commented Feb 26, 2022

giordano commented Feb 26, 2022 •

edited

Loading

DilumAluthge commented Feb 26, 2022

giordano commented Mar 7, 2022

staticfloat commented Mar 8, 2022

giordano commented Mar 10, 2022 •

edited

Loading

giordano Mar 10, 2022

vtjnash Mar 31, 2022

giordano Mar 31, 2022

imciner2 commented Mar 11, 2022

giordano commented Mar 11, 2022

imciner2 commented Mar 11, 2022

giordano commented Mar 24, 2022 •

edited

Loading

chflood commented Mar 31, 2022 via email

Silence all warnings when building with GCC on Linux #44350

Silence all warnings when building with GCC on Linux #44350

Conversation

giordano commented Feb 25, 2022 • edited Loading

giordano commented Feb 26, 2022 • edited Loading

giordano commented Feb 26, 2022

vtjnash commented Feb 26, 2022

giordano commented Feb 26, 2022 • edited Loading

DilumAluthge commented Feb 26, 2022

giordano commented Mar 7, 2022

staticfloat commented Mar 8, 2022

giordano commented Mar 10, 2022 • edited Loading

giordano Mar 10, 2022

Choose a reason for hiding this comment

vtjnash Mar 31, 2022

Choose a reason for hiding this comment

giordano Mar 31, 2022

Choose a reason for hiding this comment

imciner2 commented Mar 11, 2022

giordano commented Mar 11, 2022

imciner2 commented Mar 11, 2022

giordano commented Mar 24, 2022 • edited Loading

chflood commented Mar 31, 2022 via email

giordano commented Feb 25, 2022 •

edited

Loading

giordano commented Feb 26, 2022 •

edited

Loading

giordano commented Feb 26, 2022 •

edited

Loading

giordano commented Mar 10, 2022 •

edited

Loading

giordano commented Mar 24, 2022 •

edited

Loading