gcc 12.1 build warnings

[kallisti5]

I was playing with gcc12, and it pointed out a few new warnings we might want to investigate...

In file included from ../headers/os/kernel/OS.h:14,
                 from ../headers/private/kernel/arch/cpu.h:12,
                 from ../headers/private/kernel/arch/riscv64/arch_kernel.h:11,
                 from ../headers/private/kernel/kernel.h:14,
                 from ../headers/private/kernel/arch/riscv64/arch_thread_types.h:8,
                 from ../headers/private/kernel/arch/thread_types.h:8,
                 from ../headers/private/kernel/thread_types.h:15,
                 from ../headers/private/kernel/team.h:9,
                 from ../src/system/kernel/team.cpp:15:
In function 'int32 atomic_get(int32*)',
    inlined from 'void _user_exit_team(status_t)' at ../src/system/kernel/team.cpp:4336:17:
../headers/os/support/SupportDefs.h:307:31: error: 'unsigned int __atomic_load_4(const volatile void*, int)' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  307 |         return __atomic_load_n(value, __ATOMIC_ACQUIRE);
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

C++ objects/haiku/riscv64/release/system/kernel/lock.o 
In file included from ../headers/os/kernel/OS.h:14,
                 from ../headers/private/kernel/arch/cpu.h:12,
                 from ../headers/private/kernel/arch/riscv64/arch_kernel.h:11,
                 from ../headers/private/kernel/kernel.h:14,
                 from ../headers/private/kernel/arch/riscv64/arch_thread_types.h:8,
                 from ../headers/private/kernel/arch/thread_types.h:8,
                 from ../headers/private/kernel/thread_types.h:15,
                 from ../headers/private/kernel/team.h:9,
                 from ../src/system/kernel/team.cpp:15:
In function 'int32 atomic_get(int32*)',
    inlined from 'void _user_exit_team(status_t)' at ../src/system/kernel/team.cpp:4336:17:
../headers/os/support/SupportDefs.h:307:31: error: 'unsigned int __atomic_load_4(const volatile void*, int)' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  307 |         return __atomic_load_n(value, __ATOMIC_ACQUIRE);
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

In file included from ../headers/private/kernel/core_dump.h:11,
                 from ../src/system/kernel/debug/core_dump.cpp:7:
In function 'int32 atomic_or(int32*, int32)',
    inlined from 'status_t {anonymous}::CoreDumper::Dump(const char*, bool)' at ../src/system/kernel/debug/core_dump.cpp:787:17,
    inlined from 'status_t core_dump_write_core_file(const char*, bool)' at ../src/system/kernel/debug/core_dump.cpp:1600:25:
../headers/os/support/SupportDefs.h:300:33: error: 'unsigned int __atomic_fetch_or_4(volatile void*, unsigned int, int)' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  300 |         return __atomic_fetch_or(value, orValue, __ATOMIC_SEQ_CST);
      |                ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

In file included from ../headers/private/kernel/condition_variable.h:16,
                 from ../src/system/kernel/device_manager/IOSchedulerSimple.h:12,
                 from ../src/system/kernel/device_manager/IOSchedulerSimple.cpp:8:
In member function 'void DoublyLinkedList<Element, GetLink>::Insert(Element*, bool) [with Element = IORequestOwner; GetLink = DoublyLinkedListStandardGetLink<IORequestOwner>]',
    inlined from 'void DoublyLinkedList<Element, GetLink>::Insert(Element*, bool) [with Element = IORequestOwner; GetLink = DoublyLinkedListStandardGetLink<IORequestOwner>]' at ../headers/private/kernel/util/DoublyLinkedList.h:393:1,
    inlined from 'void DoublyLinkedList<Element, GetLink>::InsertBefore(Element*, Element*) [with Element = IORequestOwner; GetLink = DoublyLinkedListStandardGetLink<IORequestOwner>]' at ../headers/private/kernel/util/DoublyLinkedList.h:433:9,
    inlined from 'void DoublyLinkedList<Element, GetLink>::InsertBefore(Element*, Element*) [with Element = IORequestOwner; GetLink = DoublyLinkedListStandardGetLink<IORequestOwner>]' at ../headers/private/kernel/util/DoublyLinkedList.h:428:1,
    inlined from 'void DoublyLinkedList<Element, GetLink>::Insert(Element*, Element*) [with Element = IORequestOwner; GetLink = DoublyLinkedListStandardGetLink<IORequestOwner>]' at ../headers/private/kernel/util/DoublyLinkedList.h:498:14,
    inlined from 'status_t IOSchedulerSimple::_Scheduler()' at ../src/system/kernel/device_manager/IOSchedulerSimple.cpp:682:31:
../headers/private/kernel/util/DoublyLinkedList.h:410:31: error: storing the address of local variable 'marker' in '*&this_63(D)->fActiveRequestOwners.DoublyLinkedList<IORequestOwner>::fLast' [-Werror=dangling-pointer=]
  410 |                         fLast = element;
      |                         ~~~~~~^~~~~~~~~
../src/system/kernel/device_manager/IOSchedulerSimple.cpp: In member function 'status_t IOSchedulerSimple::_Scheduler()':
../src/system/kernel/device_manager/IOSchedulerSimple.cpp:592:24: note: 'marker' declared here
  592 |         IORequestOwner marker;
      |                        ^~~~~~
../src/system/kernel/device_manager/IOSchedulerSimple.cpp:592:24: note: '<unknown>' declared here
C++ objects/haiku/riscv64/release/system/kernel/disk_device_manager/KPartitioningSystem.o 
cc1plus: all warnings being treated as errors

[jessicah]

The into region of size 0 is gcc deciding that a NULL pointer is possible, and can be silenced with returning early from a NULL check.

The IOSchedulerSimple one is not so easy, I don't know how you'd solve GCC's static analysis here, since it really is putting a local variable into the list, and then later removing it. It kind of looks like a hack to avoid iterators that would get invalidated on list modification, to be able to insert into the list at a specific point.

An alternative would be to heap allocate the marker.. don't know if the local variable optimisation matters to perf, or if it was a premature optimisation, where a heap allocation isn't actually significant to perf?

[pulkomandy]

An alternative would be to heap allocate the marker.. don't know if the local variable optimisation matters to perf, or if it was a premature optimisation, where a heap allocation isn't actually significant to perf?

I would go with disabling the warning for this part of the code and add a comment explaining why it is harmless in this case.

Another option is simply statically allocating the marker (static IORequestOwner marker;) if there is no thread safety issue with doing that (I don't know, I didn't look at the code).

[jessicah]

Static doesn't work quite right, it pulls in a call to atexit, also, I didn't check if this is run in multiple threads, which would make it unsafe as you noted.

[nielx]

Haiku has updated to GCC 13. Issues like these have been fixed or mitigated.

[pulkomandy]

Have they? The ArchitectureRules jamfile still excludes these warnings from -Werror (making them simple warnings) so we can still build Haiku, but that's not really fixing the problem.

It looks like for this one we need to add attribute((nonnull)) to some variables.

[nielx]

Replying to pulkomandy:

Have they? The ArchitectureRules jamfile still excludes these warnings from -Werror (making them simple warnings) so we can still build Haiku, but that's not really fixing the problem.

It looks like for this one we need to add attribute((nonnull)) to some variables.

In the examples above we are looking at the stringop-overflow and dangling-pointer warnings. I do not see these explicitly disabled in the ArchitectureRules. Or am I missing something?

[pulkomandy]

Line 128 of the file disables errors for these warnings:

• deprecated-declarations
• cpp (misuse of the preprocessor)
• register (use of the "register" keyword)
• stringop-overread
• array-bounds
• cast-align (harmless on x86 but we would need to fix these for sparc for example)
• format-truncation

Possibly they were added for even older versions of gcc, but still, they should be cleaned. I recently removed some, I will do more.