[tz] [PROPOSED 1/7] Avoid undefined behavior if no Link lines

[Paul Eggert]

On 2022-10-26 07:09, Steve Summit via tz wrote:
>> The core dump occurred because GCC translates this:
>>     qsort(links, nlinks, sizeof *links, qsort_linkcmp);
>> as if it were this:
>>
>>     if (nlinks == 0)
>>       __builtin_trap();
>>     qsort(links, nlinks, sizeof *links, qsort_linkcmp);
> This is an astonishing result.  It's hard to imagine it's
> worthwhile for gcc to perform this "optimization".

Oh, sorry, it's astonishing because I misread the assembly language that 
GCC generates. The actual translation looks at links, not nlinks. That 
is, it's translated as if it were:

    if (links == NULL)
      __builtin_trap();
    qsort(links, nlinks, sizeof *links, qsort_linkcmp);

Sorry about the confusion.


> Paul, which version of gcc is this, and what is
> $(GCC_DEBUG_FLAGS) expanding to?

It's GCC 12.2.1 20220819 (Red Hat 12.2.1-2). $(GCC_DEBUG_FLAGS) is the 
default in the Makefile, which expands to the long string at the end of 
this email. The key option here is -fsanitize=undefined, which causes 
GCC to insert extra checking code to trap in many (but not all) places 
when behavior is undefined; this explains why GCC pessimized the code here.

GCC generates the ud2 instruction because $(GCC_DEBUG_FLAGS) also 
contains -fsanitize-undefined-trap-on-error. I use this option, despite 
the hassle it causes when debugging, because it means I don't have to 
worry about libubsan which can be a configuration problem.

I use these debugging options partly to check tzcode's portability. 
Although qsort(NULL, 0, ...) works fine on most practical platforms, as 
Clive noted there is (or was) an oddball platform or two that is (or 
was) verrrry picky about pointers, and for a program like zic where 
portability is more important than performance, it's nice if zic runs 
even on oddballs. Clive, do you happen to know what these platforms are 
(or were), and whether they're still supported?

As a practical matter, even though the C standard says that expressions 
like qsort(NULL, 0, ...) and (char *)NULL + 0 have undefined behavior, 
any new platform would be verrrry wise to define them to work the same 
way that most practical platforms do; this is true regardless of what 
the C standard says. And to some extent this means the C standard is not 
at the sweetest spot it could be in, as a contract between implementers 
and programmers. I write and see a lot of code where adding 0 to NULL is 
expected to yield NULL, and nobody thinks twice about it (nor should they).

Here's what $(GCC_DEBUG_FLAGS) expands to in the default build:

   -DTZDIR='"/usr/share/zoneinfo"' -DGCC_LINT -g3 -O3 -fno-common 
-fsanitize=undefined -fsanitize-address-use-after-scope 
-fsanitize-undefined-trap-on-error -fstack-protector -Wall -Wextra 
-Walloc-size-larger-than=100000 -Warray-bounds=2 -Wbad-function-cast 
-Wbidi-chars=any,ucn -Wcast-align=strict -Wdate-time 
-Wdeclaration-after-statement -Wdouble-promotion -Wduplicated-branches 
-Wduplicated-cond -Wformat=2 -Wformat-overflow=2 -Wformat-signedness 
-Wformat-truncation -Winit-self -Wlogical-op -Wmissing-declarations 
-Wmissing-prototypes -Wnested-externs -Wnull-dereference 
-Wold-style-definition -Woverlength-strings -Wpointer-arith -Wshadow 
-Wshift-overflow=2 -Wstrict-overflow -Wstrict-prototypes 
-Wstringop-overflow=4 -Wstringop-truncation -Wsuggest-attribute=cold 
-Wsuggest-attribute=const -Wsuggest-attribute=format 
-Wsuggest-attribute=malloc -Wsuggest-attribute=noreturn 
-Wsuggest-attribute=pure -Wtrampolines -Wundef -Wuninitialized 
-Wunused-macros -Wuse-after-free=3 -Wvariadic-macros -Wvla 
-Wwrite-strings -Wno-address -Wno-format-nonliteral -Wno-sign-compare 
-Wno-type-limits -Wno-unused-parameter