clang/limits.h: Avoid including limits.h twice #120526

wzssyqa · 2024-12-19T06:22:11Z

The limits.h of glibc, aka /usr/include/limits.h file of *-linux-gnu systems, has #include_next <limits.h>, so the limits.h from clang is included.

And in the limits.h for clang, #include_next <limits.h> is also used.

Normally it won't be a problem as the headers is protected by something like _LIBC_LIMITS_H_, while it may be a problem when we cross-build glibc on a none glibc platform. For example if we build glibc for x86_64-linux-gnu on a x86_64-linux-musl platform.

To test it, we can do

echo "#include </usr/include/limits.h>" | clang-19 -E -O2 -xc - -MM -H

We can see there is

. /usr/include/limits.h
.. /usr/lib/llvm-19/lib/clang/19/include/limits.h
... /usr/include/limits.h

It seems that other libc implementations other than glibc don't have this problem.

llvmbot · 2024-12-19T06:22:47Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: YunQiang Su (wzssyqa)

Changes

The limits.h of glibc, aka /usr/include/limits.h file of *-linux-gnu systems, has #include_next <limits.h>, so the limits.h from clang is included.

And in the limits.h for clang, #include_next <limits.h> is also used.

Normally it won't be a problem as the headers is protected by something like _LIBC_LIMITS_H_, while it may be a problem when we cross-build glibc on a none glibc platform. For example if we build glibc for x86_64-linux-gnu on a x86_64-linux-musl platform.

To test it, we can do

echo "#include &lt;/usr/include/limits.h&gt;" | bin/clang -E -O2 -xc - -MM -H

We can see there is

. /usr/include/limits.h
.. /usr/lib/llvm-19/lib/clang/19/include/limits.h
... /usr/include/limits.h

Full diff: https://github.com/llvm/llvm-project/pull/120526.diff

1 Files Affected:

(modified) clang/lib/Headers/limits.h (+3-2)

diff --git a/clang/lib/Headers/limits.h b/clang/lib/Headers/limits.h
index d08227fe4d3d48..90f01791f857ee 100644
--- a/clang/lib/Headers/limits.h
+++ b/clang/lib/Headers/limits.h
@@ -20,8 +20,9 @@
 #endif
 
 /* System headers include a number of constants from POSIX in <limits.h>.
-   Include it if we're hosted. */
-#if __STDC_HOSTED__ && __has_include_next(<limits.h>)
+   Include it if we're hosted. The limits.h of glibc uses include_next to
+   include us, we shouldn't include it again. */
+#if __STDC_HOSTED__ && __has_include_next(<limits.h>) && !defined(_LIBC_LIMITS_H_)
 #include_next <limits.h>
 #endif

wzssyqa · 2025-01-07T08:29:07Z

ping

pirama-arumuga-nainar · 2025-01-07T18:11:11Z

Does this pattern still occur while doing echo "#include <limits.h>" | clang-19 -E -O2 -xc - -MM -H? That is idiomatic compared to including /usr/include/limits.h directly.

It also doesn't seem correct to have clang's headers depend on the _LIBC_LIMITS_H_ macro which is defined in glibc.

Adding @AaronBallman who will have stronger opinion around this.

AaronBallman · 2025-01-07T19:08:49Z

I'm confused why this is necessary, we already work around glibc's quirk here:

llvm-project/clang/lib/Headers/limits.h

Line 18 in a15fedc

#if defined __GNUC__ && !defined _GCC_LIMITS_H_

which should then hit glibc's header guard here:
https://sourceware.org/git/?p=glibc.git;a=blob;f=include/limits.h#l122

wzssyqa · 2025-01-08T01:32:33Z

I'm confused why this is necessary, we already work around glibc's quirk here:

llvm-project/clang/lib/Headers/limits.h

Line 18 in a15fedc

#if defined __GNUC__ && !defined _GCC_LIMITS_H_

which should then hit glibc's header guard here:
https://sourceware.org/git/?p=glibc.git;a=blob;f=include/limits.h#l122

There are two problems:

it makes the unittests of glibc unhappy when I am using clang to build glibc.
If we build glibc on an none-glibc system, such as x86_64-linux-musl.
- #include </absolute/path/limits.h> import /usr/lib/llvm-19/lib/clang/19/include/limits.h
- /usr/lib/llvm-19/lib/clang/19/include/limits.h import /usr/include/limits.h
- /usr/include/limits.h is from musl, which is not protected and it define duplicated macro with </absolute/path/limits.h>

AaronBallman · 2025-01-10T14:25:02Z

Maybe my brain still isn't engaging after the holidays, but I'm still not seeing why this change is correct. From the point when Clang gets involved:

The absolute path includes glibc's limits.h, which includes the next header in the chain, which is Clang's: https://sourceware.org/git/?p=glibc.git;a=blob;f=include/limits.h#l122
If _GCC_LIMITS_H is not defined (but it is in this case), we define it:

llvm-project/clang/lib/Headers/limits.h

Line 18 in 6b12272

#if defined __GNUC__ && !defined _GCC_LIMITS_H_
Then, if we're hosted and there's another limits.h, we include it again:

llvm-project/clang/lib/Headers/limits.h

Line 24 in 6b12272

#if __STDC_HOSTED__ && __has_include_next(<limits.h>)
This goes on to find musl's header which doesn't protect against being included this way. But your changes don't impact that situation because it is avoiding the include when _LIBC_LIMITS_H is defined, but musl uses _LIMITS_H: https://elixir.bootlin.com/musl/v1.2.5/source/include/limits.h#L1
What's more, glibc's use of _LIBC_LIMITS_H ends before doing the include_next: https://sourceware.org/git/?p=glibc.git;a=blob;f=include/limits.h#l115

So as best I can tell, this is basically another variant of the same thing we were already doing to avoid re-including (part of) glibc, which is why I don't see how that helps. I would have expected the changes to be something more along the lines of (before the block where we define _GCC_LIMITS_H):

/* When building glibc on a musl libc system, glibc will include
   Clang's limits.h, which will include musl's limits.h, which
   cannot handle being included that way. Avert this #include_next
   madness by skipping musl's header. */
#if defined __GNUC__ && defined _GCC_LIMITS_H_ && !defined _LIMITS_H
#define _LIMITS_H
#endif

but that leads to the question of: why is including musl's limits.h a problem? It redefines macros, but that is fine on its own: https://godbolt.org/z/4nGf8zzxE If glibc is expecting different values, that seems like glibc could run into the issue if musl's header was discoverable on the include paths without involving Clang.

wzssyqa · 2025-01-14T04:07:03Z

You are right. My patch only fix the first problem: making a unit test of glibc happy.
The failure test case of glibc is:

check-local-headers

See: #120673 also.

wzssyqa · 2025-01-17T07:39:49Z

but that leads to the question of: why is including musl's limits.h a problem?

If we build glibc on Alpine with clang, we will meet the problem like

./include/bits/xopen_lim.h:84:10: warning: "NL_NMAX" redefined
   84 | # define NL_NMAX        INT_MAX

It's due to that

    glibc/include/limits.h  includes
        /usr/lib/llvm19/lib/clang/19/include/limits.h  includes
            /usr/include/limits.h

The limits.h of glibc, aka /usr/include/limits.h file of *-linux-gnu systems, has `#include_next <limits.h>`, so the limits.h from clang is included. And in the limits.h for clang, `#include_next <limits.h>` is also used. Normally it won't be a problem as the headers is protected by something like _LIBC_LIMITS_H_, while it may be a problem when we cross-build glibc on a none glibc platform. For example if we build glibc for x86_64-linux-gnu on a x86_64-linux-musl platform. To test it, we can do ``` echo "#include </usr/include/limits.h>" | bin/clang -E -O2 -xc - -MM -H ``` We can see there is ``` . /usr/include/limits.h .. /usr/lib/llvm-19/lib/clang/19/include/limits.h ... /usr/include/limits.h ```

AaronBallman · 2025-01-17T13:11:05Z

but that leads to the question of: why is including musl's limits.h a problem?

If we build glibc on Alpine with clang, we will meet the problem like
./include/bits/xopen_lim.h:84:10: warning: "NL_NMAX" redefined
   84 | # define NL_NMAX        INT_MAX
It's due to that
    glibc/include/limits.h  includes
        /usr/lib/llvm19/lib/clang/19/include/limits.h  includes
            /usr/include/limits.h

Yes and no. It's due to the redefinitions, for sure. But that diagnostic is suppressed in system headers: https://godbolt.org/z/Mb7Kh975f, so I think we need to understand why there's a limits.h being included as though it were not a system header, because that's a bug.

wzssyqa · 2025-01-21T01:30:04Z

Yes and no. It's due to the redefinitions, for sure. But that diagnostic is suppressed in system headers: https://godbolt.org/z/Mb7Kh975f, so I think we need to understand why there's a limits.h being included as though it were not a system header, because that's a bug.

As we are building glibc, in which it includes the limits.h of itself, instead of the system one.

AaronBallman · 2025-01-21T14:54:35Z

Yes and no. It's due to the redefinitions, for sure. But that diagnostic is suppressed in system headers: https://godbolt.org/z/Mb7Kh975f, so I think we need to understand why there's a limits.h being included as though it were not a system header, because that's a bug.

As we are building glibc, in which it includes the limits.h of itself, instead of the system one.

That's the bug then. It needs to include the header as a system header, not a user header.

wzssyqa · 2025-01-22T02:34:21Z

Yes and no. It's due to the redefinitions, for sure. But that diagnostic is suppressed in system headers: https://godbolt.org/z/Mb7Kh975f, so I think we need to understand why there's a limits.h being included as though it were not a system header, because that's a bug.

As we are building glibc, in which it includes the limits.h of itself, instead of the system one.

That's the bug then. It needs to include the header as a system header, not a user header.

Yes. It does. It uses -I option to include limits.h of itself.
The problem is that
limits.h of glibc includes
limits.h of clang
and
limits.h of clang includes
limits.h of system

AaronBallman · 2025-01-22T13:19:38Z

Yes and no. It's due to the redefinitions, for sure. But that diagnostic is suppressed in system headers: https://godbolt.org/z/Mb7Kh975f, so I think we need to understand why there's a limits.h being included as though it were not a system header, because that's a bug.

As we are building glibc, in which it includes the limits.h of itself, instead of the system one.

That's the bug then. It needs to include the header as a system header, not a user header.

Yes. It does. It uses -I option to include limits.h of itself.

Do you get the same behavior when you use -isystem instead?

wzssyqa · 2025-01-24T05:19:27Z

I have a test like with file

/*k.c */
#include <limits.h>
CHAR_BIT

/* include/limits.h */
#ifndef _LIBC_LIMITS_H_
#define _LIBC_LIMITS_H_
#include_next <limits.h>
#define CHAR_BIT 9
#endif

The commands

clang -E -MF - -MM -H -isystem include k.c
clang -E -MF - -MM -H -I include k.c

emit the same error message

. include/limits.h
.. /usr/lib/llvm-19/lib/clang/19/include/limits.h
... /usr/include/limits.h
In file included from k.c:1:
In file included from include/limits.h:3:
In file included from /usr/lib/llvm-19/lib/clang/19/include/limits.h:25:
/usr/include/limits.h:145:5: error: function-like macro '__GLIBC_USE' is not defined
  145 | #if __GLIBC_USE (IEC_60559_BFP_EXT_C23)
      |     ^
/usr/include/limits.h:184:5: error: function-like macro '__GLIBC_USE' is not defined
  184 | #if __GLIBC_USE (ISOC23)
      |     ^
k.o: k.c include/limits.h /usr/lib/llvm-19/lib/clang/19/include/limits.h \
  /usr/include/limits.h
2 errors generated.

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Dec 19, 2024

wzssyqa removed the backend:X86 label Dec 19, 2024

wzssyqa requested a review from pirama-arumuga-nainar December 19, 2024 06:23

pirama-arumuga-nainar requested a review from AaronBallman January 7, 2025 18:11

wzssyqa force-pushed the clang-limits-h branch from a7e475b to cc30e98 Compare January 17, 2025 07:49

llvmbot added the backend:X86 label Jan 17, 2025

clang/limits.h: Avoid including limits.h twice #120526

Are you sure you want to change the base?

clang/limits.h: Avoid including limits.h twice #120526

Uh oh!

Conversation

wzssyqa commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wzssyqa commented Jan 7, 2025

Uh oh!

pirama-arumuga-nainar commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Jan 7, 2025

Uh oh!

wzssyqa commented Jan 8, 2025

Uh oh!

AaronBallman commented Jan 10, 2025

Uh oh!

wzssyqa commented Jan 14, 2025

Uh oh!

wzssyqa commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Jan 17, 2025

Uh oh!

wzssyqa commented Jan 21, 2025

Uh oh!

AaronBallman commented Jan 21, 2025

Uh oh!

wzssyqa commented Jan 22, 2025

Uh oh!

AaronBallman commented Jan 22, 2025

Uh oh!

wzssyqa commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wzssyqa commented Dec 19, 2024 •

edited

Loading

llvmbot commented Dec 19, 2024 •

edited

Loading

pirama-arumuga-nainar commented Jan 7, 2025 •

edited

Loading

wzssyqa commented Jan 17, 2025 •

edited

Loading

wzssyqa commented Jan 24, 2025 •

edited

Loading