Skip to content

[clang][driver] Cleanup UEFI toolchain driver #111473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 22, 2024

Conversation

Prabhuk
Copy link
Contributor

@Prabhuk Prabhuk commented Oct 8, 2024

Updating UEFI header includes to not include system include directories, adding includes from compiler resource directory and minor cleanups.

Copy link

github-actions bot commented Oct 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

UEFI toolchain predefines clean up. Introducing new __PECOFF__ predefine
for targets that produce PE COFF binaries. Updating UEFI header includes
to not include system include directories.
@Prabhuk Prabhuk force-pushed the update_uefi_driver branch from c241379 to 9b55509 Compare October 9, 2024 04:13
@Prabhuk Prabhuk marked this pull request as ready for review October 9, 2024 17:10
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 9, 2024
@Prabhuk Prabhuk requested a review from petrhosek October 9, 2024 17:11
@llvmbot
Copy link
Member

llvmbot commented Oct 9, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Prabhuk (Prabhuk)

Changes

Updating UEFI header includes to not include system include directories, adding includes from compiler resource directory and minor cleanups.


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

4 Files Affected:

  • (modified) clang/lib/Basic/Targets/X86.h (-5)
  • (modified) clang/lib/Driver/ToolChains/UEFI.cpp (+18)
  • (modified) clang/lib/Driver/ToolChains/UEFI.h (+4)
  • (modified) clang/lib/Lex/InitHeaderSearch.cpp (+1)
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index a99ae62984c7d5..de371743481144 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -826,11 +826,6 @@ class LLVM_LIBRARY_VISIBILITY UEFIX86_64TargetInfo
                           "i64:64-i128:128-f80:128-n8:16:32:64-S128");
   }
 
-  void getTargetDefines(const LangOptions &Opts,
-                        MacroBuilder &Builder) const override {
-    getOSDefines(Opts, X86TargetInfo::getTriple(), Builder);
-  }
-
   BuiltinVaListKind getBuiltinVaListKind() const override {
     return TargetInfo::CharPtrBuiltinVaList;
   }
diff --git a/clang/lib/Driver/ToolChains/UEFI.cpp b/clang/lib/Driver/ToolChains/UEFI.cpp
index 66cbbec59246c0..a9d7e7892c5a64 100644
--- a/clang/lib/Driver/ToolChains/UEFI.cpp
+++ b/clang/lib/Driver/ToolChains/UEFI.cpp
@@ -35,6 +35,24 @@ UEFI::UEFI(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
 
 Tool *UEFI::buildLinker() const { return new tools::uefi::Linker(*this); }
 
+void UEFI::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
+                                     ArgStringList &CC1Args) const {
+  if (DriverArgs.hasArg(options::OPT_nostdinc))
+    return;
+
+  if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
+    SmallString<128> Dir(getDriver().ResourceDir);
+    llvm::sys::path::append(Dir, "include");
+    addSystemInclude(DriverArgs, CC1Args, Dir.str());
+  }
+
+  if (DriverArgs.hasArg(options::OPT_nostdlibinc))
+    return;
+
+  if (std::optional<std::string> Path = getStdlibIncludePath())
+    addSystemInclude(DriverArgs, CC1Args, *Path);
+}
+
 void tools::uefi::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                        const InputInfo &Output,
                                        const InputInfoList &Inputs,
diff --git a/clang/lib/Driver/ToolChains/UEFI.h b/clang/lib/Driver/ToolChains/UEFI.h
index a126ac32db6c6c..7e038b5cb8b186 100644
--- a/clang/lib/Driver/ToolChains/UEFI.h
+++ b/clang/lib/Driver/ToolChains/UEFI.h
@@ -51,6 +51,10 @@ class LLVM_LIBRARY_VISIBILITY UEFI : public ToolChain {
     return false;
   }
   bool isPICDefaultForced() const override { return true; }
+
+  void
+  AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+                            llvm::opt::ArgStringList &CC1Args) const override;
 };
 
 } // namespace toolchains
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index 2218db15013d92..ed404333bd7b3d 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -305,6 +305,7 @@ bool InitHeaderSearch::ShouldAddDefaultIncludePaths(
   case llvm::Triple::PS5:
   case llvm::Triple::RTEMS:
   case llvm::Triple::Solaris:
+  case llvm::Triple::UEFI:
   case llvm::Triple::WASI:
   case llvm::Triple::ZOS:
     return false;

if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
SmallString<128> Dir(getDriver().ResourceDir);
llvm::sys::path::append(Dir, "include");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrhosek -- Introduced this to include headers from clang resource directory. Rest of this method implementation is copied from Baremetal driver without much thought. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine so long as it includes lib/<triple/ and lib/clang/20/lib/<triple>. Do we have an existing clang driver UEFI target test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a minimal test here: https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/uefi-constructed-args.c

This is the header search paths with this PR. My original goal of this change was to remove /usr/include and /usr/local/include from the search list.

[hi on] prabhukr@prabhukr:~/llvm-builds/b2$ ./bin/clang -E -v --target=x86_64-uefi - < /dev/null
Fuchsia clang version 20.0.0git ([email protected]:prabhuk/llvm-project.git e30c180214f2ca102ba6fcf1b542db7c5f2ac5fd)
Target: x86_64-unknown-uefi
Thread model: posix
InstalledDir: /usr/local/google/home/prabhukr/llvm-builds/b2/bin
Build config: +assertions
 (in-process)
 "/usr/local/google/home/prabhukr/llvm-builds/b2/bin/llvm" "clang" -cc1 -triple x86_64-unknown-uefi -E -disable-free -clear-ast-before-backend -main-file-name - -mrelocation-model pic -pic-level 2 -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -tune-cpu generic -debugger-tuning=gdb -fdebug-compilation-dir=/usr/local/google/home/prabhukr/llvm-builds/b2 -v -fcoverage-compilation-dir=/usr/local/google/home/prabhukr/llvm-builds/b2 -resource-dir /usr/local/google/home/prabhukr/llvm-builds/b2/lib/clang/20 -internal-isystem /usr/local/google/home/prabhukr/llvm-builds/b2/lib/clang/20/include -ferror-limit 19 -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcolor-diagnostics -faddrsig -o - -x c -
clang -cc1 version 20.0.0git based upon LLVM 20.0.0git default target x86_64-unknown-linux-gnu
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/google/home/prabhukr/llvm-builds/b2/lib/clang/20/include
End of search list.
# 1 "<stdin>"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 389 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "<stdin>" 2

@Prabhuk
Copy link
Contributor Author

Prabhuk commented Dec 20, 2024

@RossComputerGuy FYI

@Prabhuk Prabhuk requested a review from jhuber6 December 20, 2024 18:36
@Prabhuk
Copy link
Contributor Author

Prabhuk commented Dec 20, 2024

@RossComputerGuy -- I am planning to land this today. Would you like to take a look and let me know if you are ok with these clean ups? I am still not able to add you as a reviewer. Maybe something to do with getting an approval? I'll check the process docs.

Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@Prabhuk Prabhuk merged commit dce2245 into llvm:main Dec 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants