Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

systemd fails to compile with gcc with -O3 and without -flto #6119

Closed
FireBurn opened this issue Jun 13, 2017 · 30 comments
Closed

systemd fails to compile with gcc with -O3 and without -flto #6119

FireBurn opened this issue Jun 13, 2017 · 30 comments

Comments

@FireBurn
Copy link
Contributor

Submission type

  • Bug report

systemd version the issue has been seen with

233

NOTE: Do not submit bug reports about anything but the two most recently released systemd versions upstream!

Used distribution

Gentoo (gcc 7.1)

In case of bug report: Expected behaviour you didn't see

In case of bug report: Unexpected behaviour you saw

Compilation error with gcc 7.1

/var/tmp/portage/sys-apps/systemd-233-r1/work/systemd-233/src/shared/install.c: In function ‘unit_file_disable’:
/var/tmp/portage/sys-apps/systemd-233-r1/work/systemd-233/src/shared/install.c:1007:22: error: argument 1 null where non-null expected [-Werror=nonnull]
name = basename(path);
~~~~~^~~~~~~~~~~~~~~~
In file included from /var/tmp/portage/sys-apps/systemd-233-r1/work/systemd-233/src/shared/install.c:28:0:
/usr/include/string.h:599:14: note: in a call to function ‘basename’ declared here
extern char *basename (const char *__filename) __THROW __nonnull ((1));
^~~~~~~~

Full log attached

In case of bug report: Steps to reproduce the problem

Compile systemd with gcc 7.1 (gcc 6.3 works fine

)

@FireBurn
Copy link
Contributor Author

Github didn't accept .log .xz .zip files to be uploaded, renamed to .txt

build.txt

@poettering
Copy link
Member

hmpf, gcc is clearly wrong here... We explicitly check first that either "name" or "path" are non-null first. And then, if "name" is NULL, we will initialize it from basename(path). This means path must be non-NULL in that case...

I am not sure what we should do in this case. I am a bit puzzled why gcc thinks this should be an error rather than just a warning, and I wonder if there is a way how to turn this off

@keszybz
Copy link
Member

keszybz commented Jun 13, 2017

I'd guess that your gcc needs updating. gcc-7.1.1-2.fc26.x86_64 works fine.

@poettering
Copy link
Member

@FireBurn which gcc version are you using precisely? 7.1.1?

@FireBurn
Copy link
Contributor Author

GCC 7.1.0, there is no GCC 7.1.1 that I can see on gcc.gnu.org

@keszybz
Copy link
Member

keszybz commented Jun 13, 2017

Yeah, the changelog for the Fedora package is:

Fri May 26 2017 Jakub Jelinek jakub@redhat.com 7.1.1-2

  • update from the 7 branch
    • PRs ada/80626, ada/80784, documentation/50642, fortran/78659,
      fortran/80121, fortran/80333, fortran/80392, fortran/80484,
      fortran/80741, fortran/80752, go/64238, libgfortran/80333,
      libgfortran/80727, libgfortran/80741, libstdc++/78939, libstdc++/80478,
      libstdc++/80761, libstdc++/80796, middle-end/80539, middle-end/80809,
      middle-end/80853, rtl-optimization/80754, sanitizer/80659,
      sanitizer/80875, target/68163, target/79027, target/79202,
      target/79203, target/80090, target/80510, target/80671, target/80706,
      target/80799, tree-optimization/80453, tree-optimization/80492
  • fix s390 indirect_jump reloading (#1450353, PR target/80725)

Wed May 03 2017 Jakub Jelinek jakub@redhat.com 7.1.1-1

  • update from the 7 branch
    • GCC 7.1 release
    • PRs bootstrap/80531, c++/80534, c/80468, target/68491, target/79430,
      target/80530, tree-optimization/80591

So either the issue is fixed by one the the listed PRs, or you're using different compilation options that somehow confuse gcc or something else is happening. It'd be great if you could reproduce this in a Fedora chroot — gentoo is a bit too complicated ;)

@FireBurn
Copy link
Contributor Author

If you can point me to the source code for GCC 7.1.1 then I'll give it a go, but there's no sign of it on any of the gcc.gnu.org mirrors. Is this a Red Hat only thing I don't remember seeing any patch releases (only major and minor) since 5.1

@keszybz
Copy link
Member

keszybz commented Jun 14, 2017

Yes, as shown in the changelog excerpt above, it contains backports of multiple upstream patches.
The source rpm: https://kojipkgs.fedoraproject.org//packages/gcc/7.1.1/2.fc26/src/gcc-7.1.1-2.fc26.src.rpm.

@poettering
Copy link
Member

OK, closing this now, let's treat that as bug in gcc, that got fixed in gcc shortly after the release apparently, and downstream distros need to backport that, which Fedora already did apparently.

I hope that makes sense.

@yurovsky
Copy link

Did anyone find what GCC commit fixes this issue or what patch can be backported? I don't see anything relevant in any of the Fedora logs mentioned (none of the bugzilla IDs point to this issue, etc) so I don't know where it's actually fixed. I would like to backport the fix to my GCC 7.1 toolchain if possible.

@konstantinblaesi
Copy link

This still happens with gcc 7.2.0 and -O3.
It doesn't happen with -O2.
PR #6657 fixes this for me.

@MilhouseVH
Copy link
Contributor

Sorry for the "me too", but confirming the post from @konstantinblaesi - this is still an issue with gcc-7.2.0 and -O3, and #6657 fixes it.

@keszybz keszybz reopened this Sep 4, 2017
keszybz added a commit to keszybz/systemd that referenced this issue Sep 4, 2017
Seems to be some kind of confusion in gcc. Insteading of playing whack-a-mole and
adding work-arounds in code, let's adjust the compilation options instead.

Fixes systemd#6119, replaces systemd#6657.
poettering pushed a commit that referenced this issue Sep 4, 2017
Seems to be some kind of confusion in gcc. Insteading of playing whack-a-mole and
adding work-arounds in code, let's adjust the compilation options instead.

Fixes #6119, replaces #6657.
andir pushed a commit to andir/systemd that referenced this issue Sep 7, 2017
Seems to be some kind of confusion in gcc. Insteading of playing whack-a-mole and
adding work-arounds in code, let's adjust the compilation options instead.

Fixes systemd#6119, replaces systemd#6657.
andir pushed a commit to andir/systemd that referenced this issue Sep 22, 2017
Seems to be some kind of confusion in gcc. Insteading of playing whack-a-mole and
adding work-arounds in code, let's adjust the compilation options instead.

Fixes systemd#6119, replaces systemd#6657.
evverx added a commit to evverx/systemd that referenced this issue May 5, 2020
Having taken a look at https://github.com/systemd/systemd/runs/645252074?check_suite_focus=true
where fuzz-journal-remote failed with
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==16==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f864f98948e bp 0x7ffde5c6b7c0 sp 0x7ffde5c6b560 T0)
==16==The signal is caused by a READ memory access.
==16==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f864f98948e in output_short /work/build/../../src/systemd/src/shared/logs-show.c
    #1 0x7f864f984624 in show_journal_entry /work/build/../../src/systemd/src/shared/logs-show.c:1154:15
    #2 0x7f864f984b63 in show_journal /work/build/../../src/systemd/src/shared/logs-show.c:1239:21
    #3 0x4cabab in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:67:21
    #4 0x51fd16 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:556:15
    #5 0x51c330 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:470:3
    #6 0x523700 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:765:7
    #7 0x5246cd in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:792:3
    #8 0x4de3d1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:824:6
    #9 0x4cfb47 in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #10 0x7f864e69782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x41f2a8 in _start (out/fuzz-journal-remote+0x41f2a8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /work/build/../../src/systemd/src/shared/logs-show.c in output_short
==16==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x44,0x3d,0xa,0x5f,0x5f,0x52,0x45,0x41,0x4c,0x54,0x49,0x4d,0x45,0x5f,0x54,0x49,0x4d,0x45,0x53,0x54,0x41,0x4d,0x50,0x3d,0x31,0xa,0xa,
D=\x0a__REALTIME_TIMESTAMP=1\x0a\x0a
artifact_prefix='./'; Test unit written to ./crash-d635b9dd31cceff3c912fd45e1a58d7e90f0ad73
Base64: RD0KX19SRUFMVElNRV9USU1FU1RBTVA9MQoK
```
I was wondering why it hadn't been caught by the compiler even though clang should have failed to compile it with
```
../src/shared/logs-show.c:624:25: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                        print_multiline(f, 4 + fieldlen + 1, 0, OUTPUT_FULL_WIDTH, 0, false,
                        ^
../src/shared/logs-show.c:161:24: note: callee declares array parameter as static here
                size_t highlight[static 2]) {
                       ^        ~~~~~~~~~~
../src/shared/logs-show.c:1239:21: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                r = show_journal_entry(f, j, mode, n_columns, flags, NULL, NULL, ellipsized);
                    ^                                                      ~~~~
../src/shared/logs-show.c:1133:30: note: callee declares array parameter as static here
                const size_t highlight[static 2],
                             ^        ~~~~~~~~~~
2 warnings generated.
```

Given that judging by systemd#13039 it doesn't seem to be
the first time issues like that have been missed I think it would be better to turn nonnull on
and get around false positives on a case-by-case basis with DISABLE_WARNING_NONNULL .. REENABLE_WARNING

Reopens systemd#6119
keszybz pushed a commit that referenced this issue May 6, 2020
Having taken a look at https://github.com/systemd/systemd/runs/645252074?check_suite_focus=true
where fuzz-journal-remote failed with
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==16==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f864f98948e bp 0x7ffde5c6b7c0 sp 0x7ffde5c6b560 T0)
==16==The signal is caused by a READ memory access.
==16==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f864f98948e in output_short /work/build/../../src/systemd/src/shared/logs-show.c
    #1 0x7f864f984624 in show_journal_entry /work/build/../../src/systemd/src/shared/logs-show.c:1154:15
    #2 0x7f864f984b63 in show_journal /work/build/../../src/systemd/src/shared/logs-show.c:1239:21
    #3 0x4cabab in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:67:21
    #4 0x51fd16 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:556:15
    #5 0x51c330 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:470:3
    #6 0x523700 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:765:7
    #7 0x5246cd in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:792:3
    #8 0x4de3d1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:824:6
    #9 0x4cfb47 in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #10 0x7f864e69782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x41f2a8 in _start (out/fuzz-journal-remote+0x41f2a8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /work/build/../../src/systemd/src/shared/logs-show.c in output_short
==16==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x44,0x3d,0xa,0x5f,0x5f,0x52,0x45,0x41,0x4c,0x54,0x49,0x4d,0x45,0x5f,0x54,0x49,0x4d,0x45,0x53,0x54,0x41,0x4d,0x50,0x3d,0x31,0xa,0xa,
D=\x0a__REALTIME_TIMESTAMP=1\x0a\x0a
artifact_prefix='./'; Test unit written to ./crash-d635b9dd31cceff3c912fd45e1a58d7e90f0ad73
Base64: RD0KX19SRUFMVElNRV9USU1FU1RBTVA9MQoK
```
I was wondering why it hadn't been caught by the compiler even though clang should have failed to compile it with
```
../src/shared/logs-show.c:624:25: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                        print_multiline(f, 4 + fieldlen + 1, 0, OUTPUT_FULL_WIDTH, 0, false,
                        ^
../src/shared/logs-show.c:161:24: note: callee declares array parameter as static here
                size_t highlight[static 2]) {
                       ^        ~~~~~~~~~~
../src/shared/logs-show.c:1239:21: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                r = show_journal_entry(f, j, mode, n_columns, flags, NULL, NULL, ellipsized);
                    ^                                                      ~~~~
../src/shared/logs-show.c:1133:30: note: callee declares array parameter as static here
                const size_t highlight[static 2],
                             ^        ~~~~~~~~~~
2 warnings generated.
```

Given that judging by #13039 it doesn't seem to be
the first time issues like that have been missed I think it would be better to turn nonnull on
and get around false positives on a case-by-case basis with DISABLE_WARNING_NONNULL .. REENABLE_WARNING

Reopens #6119
@evverx
Copy link
Member

evverx commented May 6, 2020

Now that #15704 is merged I think this should be reopened. If it's still reproducible with the latest gcc as a last resort we can always add something like


if cc.get_id() == 'gcc'
        possible_cc_flags += [
                '-Wno-error=nonnull',
        ]
endif

@evverx evverx reopened this May 6, 2020
@evverx
Copy link
Member

evverx commented May 6, 2020

Turns out systemd still fails to compile with gcc-9.3.1 with -O3 with

In function 'install_info_add',
    inlined from 'unit_file_disable' at ../src/shared/install.c:2620:21:
../src/shared/install.c:1019:24: warning: argument 1 null where non-null expected [-Wnonnull]
 1019 |                 name = basename(path);
      |                        ^~~~~~~~~~~~~~
In file included from ../src/basic/alloc-util.h:7,
                 from ../src/shared/install.c:12:
../src/shared/install.c: In function 'unit_file_disable':
/usr/include/string.h:487:14: note: in a call to function 'basename' declared here
  487 | extern char *basename (const char *__filename) __THROW __nonnull ((1));
      |              ^~~~~~~~
...
In function 'manager_load_unit_prepare',
    inlined from 'manager_load_unit' at ../src/core/manager.c:2037:13,
    inlined from 'manager_add_job_by_name' at ../src/core/manager.c:1820:13:
../src/core/manager.c:1981:24: warning: argument 1 null where non-null expected [-Wnonnull]
 1981 |                 name = basename(path);
      |                        ^~~~~~~~~~~~~~
In file included from ../src/systemd/sd-id128.h:21,
                 from ../src/systemd/sd-messages.h:20,
                 from ../src/core/manager.c:19:
../src/core/manager.c: In function 'manager_add_job_by_name':
/usr/include/string.h:487:14: note: in a call to function 'basename' declared here
  487 | extern char *basename (const char *__filename) __THROW __nonnull ((1));
      |              ^~~~~~~~

I've just opened #15725. I hope nobody passes -O3 via CFLAGS or -Dc_args.

@evverx evverx changed the title Compilation issues with GCC 7.1 systemd fails to compile with gcc with -O3 May 6, 2020
@FireBurn
Copy link
Contributor Author

FireBurn commented May 6, 2020

I'm building systemd 245 with -O3 here on Gentoo, using GCC 9.3.0 with no issues

@evverx
Copy link
Member

evverx commented May 6, 2020

@FireBurn did you build it with 7f3a5eb?

@FireBurn
Copy link
Contributor Author

FireBurn commented May 6, 2020

I've just added that patch to the ebuild and re-emerged just fine

@FireBurn
Copy link
Contributor Author

FireBurn commented May 6, 2020

The live ebuild from current git compiles just fine for me too

@evverx
Copy link
Member

evverx commented May 6, 2020

That's certainly good news given that I'd prefer not to carry #15725. That said, I've just tried to compile systemd with gcc-9.3.1 on Fedora 31 and it failed there with:

[323/1635] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o'.
FAILED: src/shared/5afaae1@@systemd-shared-245@sta/install.c.o
cc -Isrc/shared/5afaae1@@systemd-shared-245@sta -Isrc/shared -I../src/shared -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/resolve -I../src/resolve -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/core -I../src/core -Isrc/shutdown -I../src/shutdown -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I. -I../ -I/usr/include/blkid -I/usr/include/libmount -I/usr/include/p11-kit-1 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O3 -g -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -ffunction-sections -fdata-sections -Werror=shadow -include config.h -fPIC -pthread -fvisibility=default -MD -MQ 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o' -MF 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o.d' -o 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o' -c ../src/shared/install.c
In function ‘install_info_add’,
    inlined from ‘unit_file_disable’ at ../src/shared/install.c:2620:21:
../src/shared/install.c:1019:24: error: argument 1 null where non-null expected [-Werror=nonnull]
 1019 |                 name = basename(path);
      |                        ^~~~~~~~~~~~~~
In file included from ../src/basic/alloc-util.h:7,
                 from ../src/shared/install.c:12:
../src/shared/install.c: In function ‘unit_file_disable’:
/usr/include/string.h:487:14: note: in a call to function ‘basename’ declared here
  487 | extern char *basename (const char *__filename) __THROW __nonnull ((1));
      |              ^~~~~~~~
cc1: some warnings being treated as errors
[325/1635] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/json.c.o'.
ninja: build stopped: subcommand failed.
make: *** [Makefile:2: all] Error 1
$ gcc --version
gcc (GCC) 9.3.1 20200408 (Red Hat 9.3.1-2)
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[vagrant@hey systemd]$ gcc --version
gcc (GCC) 9.3.1 20200408 (Red Hat 9.3.1-2)
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@benjarobin
Copy link
Contributor

benjarobin commented May 6, 2020

Well the compiler is not clever enough, and since the assert does nothing when NDEBUG is defined, this does not help the compiler. Maybe try to replace assert() by assert_se() ?

From unit_file_disable() the compiler cannot know that *i (the name) is not null, so the compiler must assume that *i can be null. So the condition if (!name) can be true.

Edit: Well no, *i cannot be null since STRV_FOREACH check for it... I really does not understand

@floppym
Copy link
Contributor

floppym commented May 6, 2020

the assert does nothing when NDEBUG is not defined

You have that backwards: assert does nothing when NDEBUG is defined.

@benjarobin
Copy link
Contributor

You have that backwards: assert does nothing when NDEBUG is defined.

Yes I meant the opposite.

I cannot reproduce, could you past here the command line used to build install.c, mine is:

cc -Isrc/shared/5afaae1@@systemd-shared-245@sta -Isrc/shared -I../src/shared -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/resolve -I../src/resolve -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/core -I../src/core -Isrc/shutdown -I../src/shutdown -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I. -I.. -I/usr/include/blkid -I/usr/include/libmount -I/usr/include/p11-kit-1 -flto -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -std=gnu99 -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -ffunction-sections -fdata-sections -Werror=shadow -include config.h -march=x86-64 -mtune=generic -O3 -DNDEBUG=1 -fno-plt -fPIC -pthread -fvisibility=default -MD -MQ 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o' -MF 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o.d' -o 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o' -c ../src/shared/install.c

@evverx
Copy link
Member

evverx commented May 6, 2020

@benjarobin I wonder what version of gcc and what distribution you're using?

The command line seems to have been kind of hidden in my comment in the sense that you'd have to scroll right to get to the end. Here's what it looks like:

cc -Isrc/shared/5afaae1@@systemd-shared-245@sta -Isrc/shared -I../src/shared -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/resolve -I../src/resolve -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/core -I../src/core -Isrc/shutdown -I../src/shutdown -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I. -I../ -I/usr/include/blkid -I/usr/include/libmount -I/usr/include/p11-kit-1 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O3 -g -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -ffunction-sections -fdata-sections -Werror=shadow -include config.h -fPIC -pthread -fvisibility=default -MD -MQ 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o' -MF 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o.d' -o 'src/shared/5afaae1@@systemd-shared-245@sta/install.c.o' -c ../src/shared/install.c

It's also reproducible on Debian Testing with gcc-9.3.0: https://travis-ci.org/github/systemd/systemd/jobs/683778366#L2353

@benjarobin
Copy link
Contributor

benjarobin commented May 6, 2020

Oh, sorry I did not see that the command was already there... I can reproduce with your command. I am using the same version as you, gcc 9.3.0

@benjarobin
Copy link
Contributor

benjarobin commented May 6, 2020

Ok, I found why, I am building with -flto

This code with this workaround build without any error:

if (!name) {
        assert_se(path);
        name = basename(path);
}

The same hack needs to be applied in manager_load_unit_prepare()
The line assert(name || path); can be removed...

@evverx
Copy link
Member

evverx commented May 6, 2020

Ok, I found why, I am building with -flto

Thank you! Looks like we're getting somewhere :-) Up until this point I thought meson added -flto automatically when either --optimization or --buildtype=release was used. Can confirm with -flto the issue goes away.

This code with this workaround build without any error:

It was decided not to go down that road in #6657 and I generally agree with @poettering.

@evverx evverx changed the title systemd fails to compile with gcc with -O3 systemd fails to compile with gcc with -O3 and without -flto May 6, 2020
@evverx
Copy link
Member

evverx commented May 6, 2020

On the other hand, it appears at the time everybody thought that it was just a gcc glitch that could be fixed in a matter of days by just applying a patch of some kind fixing this. As far as I can see, it has never been fixed and is still reproducible with gcc-10 so it seems to be reasonable to reconsider that decision. Anyway, what @benjarobin suggested looks better than #15725 to me (and more importantly works when -O3 is passed via CFLAGS or c_args). @benjarobin could you open a PR to kick off a new round of review?

benjarobin added a commit to benjarobin/systemd that referenced this issue May 6, 2020
keszybz pushed a commit that referenced this issue May 7, 2020
@benjarobin
Copy link
Contributor

@keszybz Do you think that we can close this issue now?

@keszybz keszybz closed this as completed May 7, 2020
@evverx
Copy link
Member

evverx commented May 9, 2020

Interestingly, now that gcc-10 can get past that point systemd fails to compile with -O3 without -flto with

[437/1622] Compiling C object 'src/core/2ac6ece@@core@sta/path.c.o'
FAILED: src/core/2ac6ece@@core@sta/path.c.o
cc -Isrc/core/2ac6ece@@core@sta -Isrc/core -I../src/core -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/home -I../src/home -Isrc/shared -I../src/shared -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/resolve -I../src/resolve -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/shutdown -I../src/shutdown -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I. -I.. -I/usr/include/libmount -I/usr/include/blkid -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O3 -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -ffunction-sections -fdata-sections -Werror=shadow -include config.h -fPIC -pthread -MD -MQ 'src/core/2ac6ece@@core@sta/path.c.o' -MF 'src/core/2ac6ece@@core@sta/path.c.o.d' -o 'src/core/2ac6ece@@core@sta/path.c.o' -c ../src/core/path.c
../src/core/path.c: In function ‘path_serialize’:
../src/core/path.c:616:24: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  616 |                 (void) serialize_item_format(f, "path-spec", "%s %i %s",
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  617 |                                              path_type_to_string(s->type),
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  618 |                                              s->previous_exists,
      |                                              ~~~~~~~~~~~~~~~~~~~
  619 |                                              s->path);
      |                                              ~~~~~~~~
In function ‘path_spec_dump’,
    inlined from ‘path_dump’ at ../src/core/path.c:392:17:
../src/core/path.c:226:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  226 |         fprintf(f,
      |         ^~~~~~~~~~
  227 |                 "%s%s: %s\n",
      |                 ~~~~~~~~~~~~~
  228 |                 prefix,
      |                 ~~~~~~~
  229 |                 path_type_to_string(s->type),
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  230 |                 s->path);
      |                 ~~~~~~~~
cc1: some warnings being treated as errors

Other than that a couple of the new zero-length-bounds warnings popped up:

[304/1622] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/ethtool-util.c.o'
../src/shared/ethtool-util.c: In function ‘ethtool_get_permanent_macaddr’:
../src/shared/ethtool-util.c:260:60: warning: array subscript 5 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[]’} [-Wzero-length-bounds]
  260 |                 ret->ether_addr_octet[i] = epaddr.addr.data[i];
      |                                            ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:704:7: note: while referencing ‘data’
  704 |  __u8 data[0];
      |       ^~~~
../src/shared/ethtool-util.c: In function ‘ethtool_set_features’:
../src/shared/ethtool-util.c:488:31: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[]’} [-Wzero-length-bounds]
  488 |         len = buffer.info.data[0];
      |               ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:631:8: note: while referencing ‘data’
  631 |  __u32 data[0];
      |        ^~~~
$ gcc --version
gcc (GCC) 10.0.1 20200430 (Red Hat 10.0.1-0.14)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I'd wait for gcc-10 to settle a bit before opening an issue though.

keszybz added a commit to keszybz/systemd that referenced this issue May 9, 2020
../src/core/path.c: In function ‘path_serialize’:
../src/core/path.c:616:24: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  616 |                 (void) serialize_item_format(f, "path-spec", "%s %%i %%s",
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  617 |                                              path_type_to_string(s->type) //,
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  618 |                                              //                                             s->previous_exists,
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  619 |                                              //                                             s->path
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  620 |                 );
      |                 ~
In function ‘path_spec_dump’,
    inlined from ‘path_dump’ at ../src/core/path.c:392:17:
../src/core/path.c:226:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  226 |         fprintf(f,
      |         ^~~~~~~~~~
  227 |                 "%s%s: %s\n",
      |                 ~~~~~~~~~~~~~
  228 |                 prefix,
      |                 ~~~~~~~
  229 |                 path_type_to_string(s->type),
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  230 |                 s->path);
      |                 ~~~~~~~~

s->type should be valid here, so let's just add an assert.

For systemd#6119 (comment).
keszybz added a commit to keszybz/systemd that referenced this issue May 9, 2020
[127/1355] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/ethtool-util.c.o'
../src/shared/ethtool-util.c: In function ‘ethtool_get_permanent_macaddr’:
../src/shared/ethtool-util.c:260:60: warning: array subscript 5 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[]’} [-Wzero-length-bounds]
  260 |                 ret->ether_addr_octet[i] = epaddr.addr.data[i];
      |                                            ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:704:7: note: while referencing ‘data’
  704 |  __u8 data[0];
      |       ^~~~
../src/shared/ethtool-util.c: In function ‘ethtool_set_features’:
../src/shared/ethtool-util.c:488:31: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[]’} [-Wzero-length-bounds]
  488 |         len = buffer.info.data[0];
      |               ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:631:8: note: while referencing ‘data’
  631 |  __u32 data[0];
      |        ^~~~

The kernel should not define the length of the array, but it does. We can't fix
that, so let's use a cast to avoid the warning.

For systemd#6119 (comment).
@keszybz
Copy link
Member

keszybz commented May 9, 2020

#15762

keszybz added a commit to keszybz/systemd that referenced this issue May 11, 2020
[127/1355] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/ethtool-util.c.o'
../src/shared/ethtool-util.c: In function ‘ethtool_get_permanent_macaddr’:
../src/shared/ethtool-util.c:260:60: warning: array subscript 5 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[]’} [-Wzero-length-bounds]
  260 |                 ret->ether_addr_octet[i] = epaddr.addr.data[i];
      |                                            ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:704:7: note: while referencing ‘data’
  704 |  __u8 data[0];
      |       ^~~~
../src/shared/ethtool-util.c: In function ‘ethtool_set_features’:
../src/shared/ethtool-util.c:488:31: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[]’} [-Wzero-length-bounds]
  488 |         len = buffer.info.data[0];
      |               ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:631:8: note: while referencing ‘data’
  631 |  __u32 data[0];
      |        ^~~~

The kernel should not define the length of the array, but it does. We can't fix
that, so let's use a cast to avoid the warning.

For systemd#6119 (comment).

v2:
- use #pragma instead of a cast. It seems the cast only works in some cases, and
  gcc is "smart" enough to see beyond the cast. Unfortunately clang does not support
  this warning, so we need to do a config check whether to try to suppress.
keszybz added a commit to systemd/systemd-stable that referenced this issue May 31, 2020
../src/core/path.c: In function ‘path_serialize’:
../src/core/path.c:616:24: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  616 |                 (void) serialize_item_format(f, "path-spec", "%s %%i %%s",
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  617 |                                              path_type_to_string(s->type) //,
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  618 |                                              //                                             s->previous_exists,
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  619 |                                              //                                             s->path
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  620 |                 );
      |                 ~
In function ‘path_spec_dump’,
    inlined from ‘path_dump’ at ../src/core/path.c:392:17:
../src/core/path.c:226:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  226 |         fprintf(f,
      |         ^~~~~~~~~~
  227 |                 "%s%s: %s\n",
      |                 ~~~~~~~~~~~~~
  228 |                 prefix,
      |                 ~~~~~~~
  229 |                 path_type_to_string(s->type),
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  230 |                 s->path);
      |                 ~~~~~~~~

s->type should be valid here, so let's just add an assert.

For systemd/systemd#6119 (comment).

(cherry picked from commit 23450c8)
keszybz added a commit to systemd/systemd-stable that referenced this issue May 31, 2020
[127/1355] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/ethtool-util.c.o'
../src/shared/ethtool-util.c: In function ‘ethtool_get_permanent_macaddr’:
../src/shared/ethtool-util.c:260:60: warning: array subscript 5 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[]’} [-Wzero-length-bounds]
  260 |                 ret->ether_addr_octet[i] = epaddr.addr.data[i];
      |                                            ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:704:7: note: while referencing ‘data’
  704 |  __u8 data[0];
      |       ^~~~
../src/shared/ethtool-util.c: In function ‘ethtool_set_features’:
../src/shared/ethtool-util.c:488:31: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[]’} [-Wzero-length-bounds]
  488 |         len = buffer.info.data[0];
      |               ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:631:8: note: while referencing ‘data’
  631 |  __u32 data[0];
      |        ^~~~

The kernel should not define the length of the array, but it does. We can't fix
that, so let's use a cast to avoid the warning.

For systemd/systemd#6119 (comment).

v2:
- use #pragma instead of a cast. It seems the cast only works in some cases, and
  gcc is "smart" enough to see beyond the cast. Unfortunately clang does not support
  this warning, so we need to do a config check whether to try to suppress.

(cherry picked from commit 94c0c5b)
eworm-de pushed a commit to eworm-de/systemd that referenced this issue Jun 23, 2020
See issue systemd#6119

(cherry picked from commit 08f4685)
Yamakuzure pushed a commit to elogind/elogind that referenced this issue Aug 31, 2020
Having taken a look at https://github.com/systemd/systemd/runs/645252074?check_suite_focus=true
where fuzz-journal-remote failed with
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==16==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f864f98948e bp 0x7ffde5c6b7c0 sp 0x7ffde5c6b560 T0)
==16==The signal is caused by a READ memory access.
==16==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f864f98948e in output_short /work/build/../../src/systemd/src/shared/logs-show.c
    #1 0x7f864f984624 in show_journal_entry /work/build/../../src/systemd/src/shared/logs-show.c:1154:15
    #2 0x7f864f984b63 in show_journal /work/build/../../src/systemd/src/shared/logs-show.c:1239:21
    #3 0x4cabab in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:67:21
    #4 0x51fd16 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:556:15
    #5 0x51c330 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:470:3
    #6 0x523700 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:765:7
    #7 0x5246cd in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:792:3
    #8 0x4de3d1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:824:6
    #9 0x4cfb47 in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #10 0x7f864e69782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x41f2a8 in _start (out/fuzz-journal-remote+0x41f2a8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /work/build/../../src/systemd/src/shared/logs-show.c in output_short
==16==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x44,0x3d,0xa,0x5f,0x5f,0x52,0x45,0x41,0x4c,0x54,0x49,0x4d,0x45,0x5f,0x54,0x49,0x4d,0x45,0x53,0x54,0x41,0x4d,0x50,0x3d,0x31,0xa,0xa,
D=\x0a__REALTIME_TIMESTAMP=1\x0a\x0a
artifact_prefix='./'; Test unit written to ./crash-d635b9dd31cceff3c912fd45e1a58d7e90f0ad73
Base64: RD0KX19SRUFMVElNRV9USU1FU1RBTVA9MQoK
```
I was wondering why it hadn't been caught by the compiler even though clang should have failed to compile it with
```
../src/shared/logs-show.c:624:25: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                        print_multiline(f, 4 + fieldlen + 1, 0, OUTPUT_FULL_WIDTH, 0, false,
                        ^
../src/shared/logs-show.c:161:24: note: callee declares array parameter as static here
                size_t highlight[static 2]) {
                       ^        ~~~~~~~~~~
../src/shared/logs-show.c:1239:21: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                r = show_journal_entry(f, j, mode, n_columns, flags, NULL, NULL, ellipsized);
                    ^                                                      ~~~~
../src/shared/logs-show.c:1133:30: note: callee declares array parameter as static here
                const size_t highlight[static 2],
                             ^        ~~~~~~~~~~
2 warnings generated.
```

Given that judging by systemd/systemd#13039 it doesn't seem to be
the first time issues like that have been missed I think it would be better to turn nonnull on
and get around false positives on a case-by-case basis with DISABLE_WARNING_NONNULL .. REENABLE_WARNING

Reopens systemd/systemd#6119
Yamakuzure pushed a commit to elogind/elogind that referenced this issue Aug 31, 2020
[127/1355] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/ethtool-util.c.o'
../src/shared/ethtool-util.c: In function ‘ethtool_get_permanent_macaddr’:
../src/shared/ethtool-util.c:260:60: warning: array subscript 5 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[]’} [-Wzero-length-bounds]
  260 |                 ret->ether_addr_octet[i] = epaddr.addr.data[i];
      |                                            ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:704:7: note: while referencing ‘data’
  704 |  __u8 data[0];
      |       ^~~~
../src/shared/ethtool-util.c: In function ‘ethtool_set_features’:
../src/shared/ethtool-util.c:488:31: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[]’} [-Wzero-length-bounds]
  488 |         len = buffer.info.data[0];
      |               ~~~~~~~~~~~~~~~~^~~
In file included from ../src/shared/ethtool-util.c:5:
../src/shared/linux/ethtool.h:631:8: note: while referencing ‘data’
  631 |  __u32 data[0];
      |        ^~~~

The kernel should not define the length of the array, but it does. We can't fix
that, so let's use a cast to avoid the warning.

For systemd/systemd#6119 (comment).

v2:
- use #pragma instead of a cast. It seems the cast only works in some cases, and
  gcc is "smart" enough to see beyond the cast. Unfortunately clang does not support
  this warning, so we need to do a config check whether to try to suppress.
elvees-gerrit pushed a commit to elvees/mcom02-buildroot-external that referenced this issue Sep 14, 2020
systemd 246 (used in Buildroot 2020.08) brings back fatality of nonnull
warning. GCC 7.x produces false positive nonnull warnings and fails
to compile systemd. See systemd issue [1].
See also [2].

[1] systemd/systemd#6119
[2] systemd/systemd#15704

Issue: #MCOM02SW-1868
Change-Id: I3b6d1ab0e0c30c0bfb57f87580f275897ec144ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

9 participants