Bug 95700 - read-md.c: "missing sentinel in function call" when building gcc with musl
Summary: read-md.c: "missing sentinel in function call" when building gcc with musl
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: bootstrap (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: build
Depends on:
Blocks:
 
Reported: 2020-06-16 15:47 UTC by Ilya Leoshkevich
Modified: 2021-12-17 05:52 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
preprocessed output (64.53 KB, text/plain)
2020-06-16 16:29 UTC, Ilya Leoshkevich
Details
proposed patch (tests are running) (20.88 KB, application/mbox)
2020-06-17 18:51 UTC, Ilya Leoshkevich
Details
aarch64 native build fix (3.52 KB, application/mbox)
2020-07-22 21:47 UTC, Ilya Leoshkevich
Details
Add -std=c++11 to the aarch64 native build fix (291 bytes, patch)
2020-07-23 15:29 UTC, vvinayag
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Leoshkevich 2020-06-16 15:47:57 UTC
I'm trying to bootstrap gcc on gcc301 with --disable-multilib --build=x86_64-alpine-linux-musl. The following error occurs:

/home/iii/gcc/regtest-f8a59086423e/build/./prev-gcc/xg++ -B/home/iii/gcc/regtest-f8a59086423e/build/./prev-gcc/ -B/home/iii/gcc/regtest-f8a59086423e/install/x86_64-alpine-linux-musl/bin/ -nostdinc++ -B/home/iii/gcc/regtest-f8a59086423e/build/prev-x86_64-alpine-linux-musl/libstdc++-v3/src/.libs -B/home/iii/gcc/regtest-f8a59086423e/build/prev-x86_64-alpine-linux-musl/libstdc++-v3/libsupc++/.libs  -I/home/iii/gcc/regtest-f8a59086423e/build/prev-x86_64-alpine-linux-musl/libstdc++-v3/include/x86_64-alpine-linux-musl  -I/home/iii/gcc/regtest-f8a59086423e/build/prev-x86_64-alpine-linux-musl/libstdc++-v3/include  -I/home/iii/gcc/regtest-f8a59086423e/libstdc++-v3/libsupc++ -L/home/iii/gcc/regtest-f8a59086423e/build/prev-x86_64-alpine-linux-musl/libstdc++-v3/src/.libs -L/home/iii/gcc/regtest-f8a59086423e/build/prev-x86_64-alpine-linux-musl/libstdc++-v3/libsupc++/.libs -c   -g -O2 -fno-checking -gtoggle  -DIN_GCC     -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-error=format-diag -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common  -DHAVE_CONFIG_H  -DGENERATOR_FILE -fno-PIE -I. -Ibuild -I../../gcc -I../../gcc/build -I../../gcc/../include  -I../../gcc/../libcpp/include  \
        -o build/read-md.o ../../gcc/read-md.c
../../gcc/read-md.c: In member function ‘const char* md_reader::join_c_conditions(const char*, const char*)’:
../../gcc/read-md.c:158:58: error: missing sentinel in function call [-Werror=format=]
  158 |   result = concat ("(", cond1, ") && (", cond2, ")", NULL);
      |                                                          ^
../../gcc/read-md.c: In member function ‘void md_reader::handle_enum(file_location, bool)’:
../../gcc/read-md.c:947:58: error: missing sentinel in function call [-Werror=format=]
  947 |    value_name = concat (def->name, "_", name.string, NULL);
      |                                                          ^
../../gcc/read-md.c: In member function ‘void md_reader::handle_include(file_location)’:
../../gcc/read-md.c:1072:57: error: missing sentinel in function call [-Werror=format=]
 1072 |    pathname = concat (stackp->fname, sep, filename, NULL);
      |                                                         ^
../../gcc/read-md.c:1085:47: error: missing sentinel in function call [-Werror=format=]
 1085 |  pathname = concat (m_base_dir, filename, NULL);
      |                                               ^
cc1plus: all warnings being treated as errors

musl has the following commit: https://git.musl-libc.org/cgit/musl/commit/?id=c8a9c22173f485c8c053709e1dfa0a617cb6be1a, which suggests that C++ (as opposed to plain C) should allow plain (uintptr_t)0 as a sentinel value.

gcc wants either a pointer or __null though: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/c-family/c-common.c;h=b1379faa412e3646a443969c0067f5c4fb23e107;hb=929fd91ba975eebf9e57f7f092041271dcaf0c34#l5385

Would it be possible to allow (uintptr_t)0 as a valid sentinel value for C++? Or is it musl that is wrong here?
Comment 1 Jakub Jelinek 2020-06-16 15:55:48 UTC
Why isn't gcc's own stddef.h used rather than the musl one?
I don't think it is a good idea to allow (uintptr_t) 0 as valid sentinel.
Comment 2 Jonathan Wakely 2020-06-16 16:14:30 UTC
(In reply to Jakub Jelinek from comment #1)
> I don't think it is a good idea to allow (uintptr_t) 0 as valid sentinel.

Definitely not. (uintptr_t)0 is not a null pointer constant, and is not a valid definition of NULL for C++.
Comment 3 Jonathan Wakely 2020-06-16 16:22:39 UTC
(In reply to Ilya Leoshkevich from comment #0)
> musl has the following commit:
> https://git.musl-libc.org/cgit/musl/commit/
> ?id=c8a9c22173f485c8c053709e1dfa0a617cb6be1a, which suggests that C++ (as
> opposed to plain C) should allow plain (uintptr_t)0 as a sentinel value.

I don't understand. musl uses a null pointer constant of type long (i.e. 0L) so where does the suggestion to allow (uintptr_t)0 come from?

If C provided a UINTPTR_C macro to produce an integer literal of type uintptr_t then UINTPTR_C(0) would be a valid sentinel. I assume that doesn't exist because C doesn't need such a type (because defining NULL to (void*)0 avoids this problem).
Comment 4 Ilya Leoshkevich 2020-06-16 16:29:25 UTC
Created attachment 48740 [details]
preprocessed output

In the preprocessed output I see that gcc's stddef.h is used, but most likely `#define NULL 0L` comes from some other musl header, since musl defines it in like 8 places.
Comment 5 Ilya Leoshkevich 2020-06-16 16:35:45 UTC
I'm sorry, I should not have written (uintptr_t)0 - I just used it as a synonym for a "pointer-sized int". Would allowing 0L as a sentinel value be a reasonable thing?
Comment 6 Jonathan Wakely 2020-06-16 16:40:02 UTC
To be really safe during stage 1, GCC should not use NULL as a pointer sentinel in C++ code anyway.

The bootstrap compiler could define it to 0 or 0u, neither of which is guaranteed to be OK to pass as a varargs sentinel where a null pointer is expected. Any of (void*)0 or (void*)NULL or nullptr would be safe.
Comment 7 Ilya Leoshkevich 2020-06-17 13:31:46 UTC
Would it be OK then to replace last arguments of functions with __attribute__((sentinel)) from NULLs to nullptrs? I can make a patch for this.
Comment 8 Ilya Leoshkevich 2020-06-17 18:51:44 UTC
Created attachment 48750 [details]
proposed patch (tests are running)
Comment 9 CVS Commits 2020-07-02 10:39:36 UTC
The master branch has been updated by Ilya Leoshkevich <iii@gcc.gnu.org>:

https://gcc.gnu.org/g:d59a576b8b5e12c3a56f0262912090e2921f5daa

commit r11-1785-gd59a576b8b5e12c3a56f0262912090e2921f5daa
Author: Ilya Leoshkevich <iii@linux.ibm.com>
Date:   Mon Jun 29 20:36:03 2020 +0200

    Redefine NULL to nullptr
    
    Bootstrap with musl libc fails with numerous "missing sentinel in
    function call" errors.  This is because musl defines NULL as 0L for C++,
    but gcc requires sentinel value to be a pointer or __null.
    
    Jonathan Wakely says:
    
        To be really safe during stage 1, GCC should not use NULL as a
        pointer sentinel in C++ code anyway.
    
        The bootstrap compiler could define it to 0 or 0u, neither of which
        is guaranteed to be OK to pass as a varargs sentinel where a null
        pointer is expected.  Any of (void*)0 or (void*)NULL or nullptr
        would be safe.
    
    While it is possible to fix this by replacing NULL sentinels with
    nullptrs, such approach would generate backporting conflicts, therefore
    simply redefine NULL to nullptr at the end of system.h, where it would
    not confuse system headers.
    
    gcc/ChangeLog:
    
    2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>
    
            PR bootstrap/95700
            * system.h (NULL): Redefine to nullptr.
Comment 10 vvinayag 2020-07-09 23:33:15 UTC
(In reply to CVS Commits from comment #9)
> The master branch has been updated by Ilya Leoshkevich <iii@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:d59a576b8b5e12c3a56f0262912090e2921f5daa
> 
> commit r11-1785-gd59a576b8b5e12c3a56f0262912090e2921f5daa
> Author: Ilya Leoshkevich <iii@linux.ibm.com>
> Date:   Mon Jun 29 20:36:03 2020 +0200
> 
>     Redefine NULL to nullptr
>     
>     Bootstrap with musl libc fails with numerous "missing sentinel in
>     function call" errors.  This is because musl defines NULL as 0L for C++,
>     but gcc requires sentinel value to be a pointer or __null.
>     
>     Jonathan Wakely says:
>     
>         To be really safe during stage 1, GCC should not use NULL as a
>         pointer sentinel in C++ code anyway.
>     
>         The bootstrap compiler could define it to 0 or 0u, neither of which
>         is guaranteed to be OK to pass as a varargs sentinel where a null
>         pointer is expected.  Any of (void*)0 or (void*)NULL or nullptr
>         would be safe.
>     
>     While it is possible to fix this by replacing NULL sentinels with
>     nullptrs, such approach would generate backporting conflicts, therefore
>     simply redefine NULL to nullptr at the end of system.h, where it would
>     not confuse system headers.
>     
>     gcc/ChangeLog:
>     
>     2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>
>     
>             PR bootstrap/95700
>             * system.h (NULL): Redefine to nullptr.

I think this patch breaks arm native and aarch64 native builds when compiling gcc/gcc/genmodes.c.

gcc/gcc/genmodes.c: In function 'const char* get_mode_class(mode_data*)':

gcc/gcc/system.h:1273:14: error: 'nullptr' was not declared in this scope
 #define NULL nullptr
              ^
gcc/gcc/genmodes.c:1221:14: note: in expansion of macro 'NULL'
       return NULL;
              ^
Comment 11 Ilya Leoshkevich 2020-07-10 09:14:24 UTC
Sorry about that! I will have a look.
Comment 12 Ilya Leoshkevich 2020-07-11 11:12:32 UTC
I managed to bootstrap and regtest upstream commit 6e41c27bf549 on gcc113 farm machine.

Two questions:

- What is your system compiler version? For GCC 11, C++11 compiler is required: https://gcc.gnu.org/install/prerequisites.html

- What exactly is "native aarch64 build" - is it simply building the compiler on aarch64, or something else?
Comment 13 vvinayag 2020-07-17 22:21:38 UTC
(In reply to Ilya Leoshkevich from comment #12)
> I managed to bootstrap and regtest upstream commit 6e41c27bf549 on gcc113
> farm machine.
> 
> Two questions:
> 
> - What is your system compiler version? For GCC 11, C++11 compiler is
> required: https://gcc.gnu.org/install/prerequisites.html
> 
> - What exactly is "native aarch64 build" - is it simply building the
> compiler on aarch64, or something else?

Sorry for the delayed response:

- The system compiler is GCC 4.8.1 (which seems to have experimental support for c++11)

- Yes, I meant to say we build the compiler on an aarch64-none-linux-gnu machine.
Comment 14 Ilya Leoshkevich 2020-07-20 09:58:49 UTC
gcc113 has 4.8.4, which is a bit newer. But in any case, according to https://gcc.gnu.org/projects/cxx-status.html, gcc should support nullptr since 4.6.

Could you please post the failing compiler invocation command?

In the meantime I will build gcc 4.8.1 on gcc113 and try to build master with it.
Comment 15 vvinayag 2020-07-20 18:38:28 UTC
(In reply to Ilya Leoshkevich from comment #14)
> gcc113 has 4.8.4, which is a bit newer. But in any case, according to
> https://gcc.gnu.org/projects/cxx-status.html, gcc should support nullptr
> since 4.6.
> 
> Could you please post the failing compiler invocation command?
> 
> In the meantime I will build gcc 4.8.1 on gcc113 and try to build master
> with it.

Hi Ilya Leoshkevich

Thanks for your help. The failing compiler command is:

g++ -c -DIN_GCC      -DGENERATOR_FILE  -I. -Ibuild -I/tmp/…/src/gcc/gcc -I/tmp/…/src/gcc/gcc/build -I/tmp/…/src/gcc/gcc/../include  -I/tmp/…/src/gcc/gcc/../libcpp/include  \
        -o build/genmodes.o /tmp/…/src/gcc/gcc/genmodes.c
Comment 16 Ilya Leoshkevich 2020-07-21 22:05:28 UTC
I finally managed to reproduce this by doing `./configure --host=aarch64-none-linux-gnu` on gcc113. The problem is that `CXX_FOR_BUILD` doesn't seem to be set correctly - normally it's `g++-4.8.1 -std=gnu++11`, but in this case it's just `g++`. I'm currently trying to wrap my head around autotools build setup in order to figure out where exactly things went wrong.
Comment 17 Ilya Leoshkevich 2020-07-22 21:47:47 UTC
Created attachment 48917 [details]
aarch64 native build fix

Could you please try the attached patch? It fixed the issue for me, and survived bootstrap/regtest on x86_64.
Comment 18 vvinayag 2020-07-22 23:06:30 UTC
(In reply to Ilya Leoshkevich from comment #17)
> Created attachment 48917 [details]
> aarch64 native build fix
> 
> Could you please try the attached patch? It fixed the issue for me, and
> survived bootstrap/regtest on x86_64.

Hi Ilya,

I am testing this patch now. Will update when done.

Also, I was a bit incorrect when I mentioned that this is for building the compiler on an aarch64-none-linux-gnu machine. The build machine is actually x86_64.
BUILD: x86_64/linux
HOST: aarch64-none-linux-gnu
TARGET: aarch64-none-linux-gnu
Sorry for the confusion.
Comment 19 vvinayag 2020-07-23 15:29:11 UTC
Created attachment 48921 [details]
Add -std=c++11 to the aarch64 native build fix

This patch adds CXX="$CXX -std=c++11" to the patch provided for the aarch64 native build fix.
Comment 20 vvinayag 2020-07-23 15:39:07 UTC
(In reply to vvinayag from comment #18)
> (In reply to Ilya Leoshkevich from comment #17)
> > Created attachment 48917 [details]
> > aarch64 native build fix
> > 
> > Could you please try the attached patch? It fixed the issue for me, and
> > survived bootstrap/regtest on x86_64.
> 
> Hi Ilya,
> 
> I am testing this patch now. Will update when done.
> 
> Also, I was a bit incorrect when I mentioned that this is for building the
> compiler on an aarch64-none-linux-gnu machine. The build machine is actually
> x86_64.
> BUILD: x86_64/linux
> HOST: aarch64-none-linux-gnu
> TARGET: aarch64-none-linux-gnu
> Sorry for the confusion.

The patch did not fix the issue for me, unfortunately, because CXX_FOR_BUILD is still set to 'g++'.
But to make it work, I added the line: CXX="$CXX -std=c++11", as shown in the file I have attached.
With this additional line in both configure and configure.ac, CXX_FOR_BUILD is set to 'g++ -std=c++11', and the build succeeded for me. However, I do not know whether that is the right solution.
Comment 21 Andrew Pinski 2021-12-17 05:49:40 UTC
(In reply to vvinayag from comment #20) 
> The patch did not fix the issue for me, unfortunately, because CXX_FOR_BUILD
> is still set to 'g++'.
> But to make it work, I added the line: CXX="$CXX -std=c++11", as shown in
> the file I have attached.
> With this additional line in both configure and configure.ac, CXX_FOR_BUILD
> is set to 'g++ -std=c++11', and the build succeeded for me. However, I do
> not know whether that is the right solution.

I think the aarch64 issue is fixed with r12-872-g5116b54e4644 on the trunk.