This is an archive of the discontinued LLVM Phabricator instance.

[clang][modules] Mark builtin header 'inttypes.h' for modules
Needs RevisionPublic

Authored by benlangmuir on Aug 15 2023, 2:02 PM.

Details

Summary

Like the other enumerated builtin headers, inttypes.h can be declared by another system module.

rdar://113924039

Diff Detail

Event Timeline

benlangmuir created this revision.Aug 15 2023, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2023, 2:02 PM
benlangmuir requested review of this revision.Aug 15 2023, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2023, 2:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
iana added a subscriber: vsapsai.Aug 15 2023, 2:11 PM

Can you add @vsapsai to the reviewers please? He was looking at this one a year or two ago.

benlangmuir added a reviewer: vsapsai.

Add missing test updates: tests using the Inputs/System/usr/include should be using -internal-isystem to get the correct search path order with respect to the resource dir. The tests that were previously using -isystem were only working before because the other headers wrap their #include_next in __has_include_next, which was causing them to silently be missing these headers. With inttypes.h the include_next is unguarded, which revealed the issue. Note: even if we someday add the has_include_next guard to inttypes.h the test change is still correct.

Running the tests locally fails with

lib/clang/18/include/inttypes.h:21:15: fatal error: 'inttypes.h' file not found
   21 | #include_next <inttypes.h>
      |               ^~~~~~~~~~~~

Is it something wrong with my local configuration?

Also want to study the previous discussion on this subject.

iana requested changes to this revision.EditedSep 12 2023, 10:43 PM

I was talking to Ben about this one a little today since I ended up needing to make the same -isystem/-I -> -internal-isystem switch in some of the unit tests in D159064, and also add a clang/test/Modules/Inputs/System/usr/include/inttypes.h.

I think it's just plain not supported to put isysroot/usr/include before resource-dir/include. That said, fusing resource-dir/include/inttypes.h with isysroot/usr/include is probably going to break the projects that messed that up, whereas having the two in different modules probably won't.

So even though this change is correct, IMO it's not worth the trouble since we have to do D159064 to make the libc++ cstd header work anyway.

This revision now requires changes to proceed.Sep 12 2023, 10:43 PM