[PATCH] D131594: WORK IN PROGRESS Add Clang UEFI target to support "x86_64-unknown-uefi" triple

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:41-45
+bool UEFI::IsUnwindTablesDefault(const ArgList &Args) const {
+  // TODO(prabhukr):: What about x86_32?
+  return getArch() == llvm::Triple::x86_64 || getArch() == llvm::Triple::arm ||
+         getArch() == llvm::Triple::thumb || getArch() == llvm::Triple::aarch64;
+}
----------------
I'd hardcode and inline this for simplicity, this toolchain is only used for x86_64 and arm64 right now so now need to duplicate that here.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:48-49
+bool UEFI::isPICDefault() const {
+  return getArch() == llvm::Triple::x86_64 ||
+         getArch() == llvm::Triple::aarch64;
+}
----------------
I'd hardcode and inline this for simplicity, this toolchain is only used for x86_64 and arm64 right now so now need to duplicate that here.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:52
+
+bool UEFI::isPIEDefault(const llvm::opt::ArgList &Args) const { return false; }
+
----------------
I'd inline this.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:55-56
+bool UEFI::isPICDefaultForced() const {
+  return getArch() == llvm::Triple::x86_64 ||
+         getArch() == llvm::Triple::aarch64;
+}
----------------
I'd hardcode and inline this for simplicity, this toolchain is only used for x86_64 and arm64 right now so now need to duplicate that here.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:59
+
+bool UEFI::IsIntegratedAssemblerDefault() const { return true; }
----------------
I'd inline this.


================
Comment at: clang/lib/Driver/ToolChains/UEFI.h:22
+       const llvm::opt::ArgList &Args);
+
+  bool IsIntegratedAssemblerDefault() const override;
----------------
You also need `HasNativeLLVMSupport`, see https://github.com/llvm/llvm-project/blob/46dee4a3a3dfb372a0eaa0b4490c80be2f421f29/clang/lib/Driver/ToolChains/Fuchsia.h#L43


================
Comment at: clang/lib/Driver/ToolChains/UEFI.h:28
+  bool isPICDefaultForced() const override;
+};
+} // namespace toolchains
----------------
You'll also need to override `buildLinker` since the default implementation doesn't do anything, see https://github.com/llvm/llvm-project/blob/46dee4a3a3dfb372a0eaa0b4490c80be2f421f29/clang/lib/Driver/ToolChain.cpp#L319

I'd hardcode it to use `lld-link` for simplicty. You can use Fuchsia's implementation for inspiration (although the flags are going to be different since Fuchsia uses `ld.lld`, not `lld-link`):

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Fuchsia.h#L17-L34
https://github.com/llvm/llvm-project/blob/609b789170625277f631139c790c22d527ff1eed/clang/lib/Driver/ToolChains/Fuchsia.cpp#L31-L191


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131594/new/

https://reviews.llvm.org/D131594