This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules][HU 2/5] Support searching Header Units in user or system search paths.
ClosedPublic

Authored by iains on Mar 7 2022, 2:24 AM.

Details

Summary

This is support for the user-facing options to create importable header units
from headers in the user or system search paths (or to be given an absolute path).

This means that an incomplete header path will be passed by the driver and the
lookup carried out using the search paths present when the front end is run.

To support this, we introduce file fypes for c++-{user,system,header-unit}-header.
These terms are the same as the ones used by GCC, to minimise the differences for
tooling (and users).

The preprocessor checks for headers before issuing a warning for
"#pragma once" in a header build. We ensure that the importable header units
are recognised as headers in order to avoid such warnings.

Diff Detail

Event Timeline

iains created this revision.Mar 7 2022, 2:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 2:24 AM
iains published this revision for review.Mar 7 2022, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 4:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
iains updated this revision to Diff 413611.Mar 7 2022, 1:12 PM

amend a testcase.

I reordered and squashed two of the patches before posting, and failed to spot that a test case had
a (non-critical) dependency on functionality introduced by a later patch. Fixed by removing the
dependency.

ChuanqiXu added inline comments.Mar 8 2022, 7:05 PM
clang/include/clang/Frontend/FrontendOptions.h
157

I prefer IsHeader

252

How about getHeaderUnitKind ?

clang/lib/Frontend/CompilerInvocation.cpp
2788–2791

How do you think about the suggested changes? I feel it is less confusing.

2796–2797

Oh, now I find Header and HeaderUnit might be a little bit confusing. It looks like Header should include HeaderUnit at the first sight. It is not true. But I don't have only suggestions...

2840–2841
2856–2858

So the compiler would crash if there is multiple inputs for header unit? I feel it is not most friendly. How about emitting an error here?

clang/lib/Frontend/FrontendAction.cpp
814–816

Is it same as the suggested?

BTW, all the tests uses `clang_cc1. How should the users do with clang? Could them use -Xclang only?

iains added a comment.Mar 8 2022, 11:44 PM

BTW, all the tests uses `clang_cc1. How should the users do with clang? Could them use -Xclang only?

The driver changes are in separate patches (they will make the same user-facing interface options as used by GCC). I wanted to have the FE implementation available so that the driver has something to drive.

BTW, all the tests uses `clang_cc1. How should the users do with clang? Could them use -Xclang only?

The driver changes are in separate patches (they will make the same user-facing interface options as used by GCC). I wanted to have the FE implementation available so that the driver has something to drive.

Makes sense to me. Thanks for the explanation.

iains updated this revision to Diff 414889.Mar 12 2022, 3:18 PM
iains marked 6 inline comments as done.

rebased, address review comments.

clang/include/clang/Frontend/FrontendOptions.h
157

OK.

252

OK. I prefer more descriptive names, but they do get rather long sometimes.

clang/lib/Frontend/CompilerInvocation.cpp
2788–2791

OK,

2796–2797

We have an amount of existing handling specific to "generic headers".
(header unit = none, header = true)

The intention is that HeaderUnitKind is used to disambiguate the cases we are dealing with C+=20 header units.

I am not sure it would be worth the code churn to replace all uses of header with a HUK.

2856–2858

well the driver is supposed to catch these problems (in the case of C++20 modules mode it will generate several compile jobs, one per header).

I added an error for this tho.

clang/lib/Frontend/FrontendAction.cpp
814–816

not quite because getDirectory() is "llvm::ErrorOr<const DirectoryEntry *>" ...

ChuanqiXu accepted this revision.Mar 13 2022, 7:28 PM

LGTM except we should remove the dead error emitting.

clang/lib/Frontend/CompilerInvocation.cpp
2796–2797

OK, this is a historical problem. It makes no sense require us to fix it in this patch.

2856–2858

Oh, so if the compiler wouldn't crash in that case, I think the original assertion is acceptable. And the current error is dead code, which is bad. Let's take the original assertion.

This revision is now accepted and ready to land.Mar 13 2022, 7:28 PM
iains updated this revision to Diff 416162.Mar 17 2022, 6:22 AM
iains marked an inline comment as done.

match windows path delimiters in testcase.

clang/lib/Frontend/CompilerInvocation.cpp
2856–2858

actually, it does protect against the case that the frontend is called directly with XClang or so, so it's not dead code, and it is better for the compiler to emit a diagnostic rather than crash (if built with assertions).

ChuanqiXu added inline comments.Mar 17 2022, 8:43 PM
clang/lib/Frontend/CompilerInvocation.cpp
2856–2858

If it is not dead codes, I feel better if we could add tests to cover it.

iains updated this revision to Diff 416842.Mar 21 2022, 1:31 AM

rebased.

iains updated this revision to Diff 418079.Mar 24 2022, 4:39 PM

rebased, added a testcase for multiple inputs.