[Driver] Add -Wa, options --crel and --allow-experimental-crel #97378

MaskRay · 2024-07-02T02:45:02Z

The two options are discussed in a few comments around
#91280 (comment)

-Wa,--crel: error "-Wa,--allow-experimental-crel must be specified to use -Wa,--crel..."
-Wa,--allow-experimental-crel: no-op
-Wa,--crel,--allow-experimental-crel: enable CREL in the integrated assembler ([MC,llvm-readobj,yaml2obj] Support CREL relocation format #91280)

MIPS's little-endian n64 ABI messed up the r_info field in
relocations. While this could be fixed with CREL, my intention is to
avoid complication in assembler/linker. The implementation simply
doesn't allow CREL for MIPS.

Link: https://discourse.llvm.org/t/rfc-crel-a-compact-relocation-format-for-elf/77600

Created using spr 1.3.5-bogner

llvmbot · 2024-07-02T02:45:35Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Fangrui Song (MaskRay)

Changes

The two options are discussed in a few comments around
#91280 (comment)

-Wa,--crel: error
-Wa,--allow-experimental-crel: no-op
-Wa,--crel,--allow-experimental-crel: enable CREL in the integrated assembler (#91280)

MIPS's little-endian n64 ABI messed up the r_info field in
relocations. While this could be fixed with CREL, my intention is to
avoid complication in assembler/linker. The implementation simply
doesn't allow CREL for MIPS.

Link: https://discourse.llvm.org/t/rfc-crel-a-compact-relocation-format-for-elf/77600

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

9 Files Affected:

(modified) clang/include/clang/Basic/CodeGenOptions.def (+1)
(modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+3)
(modified) clang/include/clang/Driver/Options.td (+3)
(modified) clang/lib/CodeGen/BackendUtil.cpp (+1)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+18)
(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+21)
(added) clang/test/Driver/crel.c (+25)
(added) clang/test/Misc/cc1as-crel.s (+6)
(modified) clang/tools/driver/cc1as_main.cpp (+6)

diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index e3f6da4a84f69..25de2204f04c0 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -36,6 +36,7 @@ VALUE_CODEGENOPT(Name, Bits, Default)
 #endif
 
 CODEGENOPT(DisableIntegratedAS, 1, 0) ///< -no-integrated-as
+CODEGENOPT(Crel, 1, 0) ///< -Wa,--crel
 CODEGENOPT(RelaxELFRelocations, 1, 1) ///< -Wa,-mrelax-relocations={yes,no}
 CODEGENOPT(AsmVerbose        , 1, 0) ///< -dA, -fverbose-asm.
 CODEGENOPT(PreserveAsmComments, 1, 1) ///< -dA, -fno-preserve-as-comments.
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 1ca2cb85565a1..fa81a63c72231 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -801,6 +801,9 @@ def warn_drv_missing_multilib : Warning<
 def note_drv_available_multilibs : Note<
   "available multilibs are:%0">;
 
+def err_drv_experimental_crel : Error<
+  "-Wa,--allow-experimental-crel must be specified to use -Wa,--crel. CREL is experimental and takes a non-standard section type code">;
+
 def warn_android_unversioned_fallback : Warning<
   "using unversioned Android target directory %0 for target %1; unversioned "
   "directories will not be used in Clang 19 -- provide a versioned directory "
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 1c2b8cfeef6ce..a9a451805be70 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -7027,6 +7027,9 @@ def massembler_no_warn : Flag<["-"], "massembler-no-warn">,
 def massembler_fatal_warnings : Flag<["-"], "massembler-fatal-warnings">,
   HelpText<"Make assembler warnings fatal">,
   MarshallingInfoFlag<CodeGenOpts<"FatalWarnings">>;
+def crel : Flag<["--"], "crel">,
+  HelpText<"Enable CREL relocation format (ELF only)">,
+  MarshallingInfoFlag<CodeGenOpts<"Crel">>;
 def mrelax_relocations_no : Flag<["-"], "mrelax-relocations=no">,
     HelpText<"Disable x86 relax relocations">,
     MarshallingInfoNegativeFlag<CodeGenOpts<"RelaxELFRelocations">>;
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 22b593e8f2b7a..9ef40c692785b 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -469,6 +469,7 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
   Options.MCOptions.AsmVerbose = CodeGenOpts.AsmVerbose;
   Options.MCOptions.Dwarf64 = CodeGenOpts.Dwarf64;
   Options.MCOptions.PreserveAsmComments = CodeGenOpts.PreserveAsmComments;
+  Options.MCOptions.Crel = CodeGenOpts.Crel;
   Options.MCOptions.X86RelaxRelocations = CodeGenOpts.RelaxELFRelocations;
   Options.MCOptions.CompressDebugSections =
       CodeGenOpts.getCompressDebugSections();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 27c451c565f2b..f098320cdb4c5 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2500,6 +2500,8 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
   // arg after parsing the '-I' arg.
   bool TakeNextArg = false;
 
+  const llvm::Triple &Triple = C.getDefaultToolChain().getTriple();
+  bool Crel = false, ExperimentalCrel = false;
   bool UseRelaxRelocations = C.getDefaultToolChain().useRelaxRelocations();
   bool UseNoExecStack = false;
   const char *MipsTargetFeature = nullptr;
@@ -2623,6 +2625,12 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
                  Value == "-nocompress-debug-sections" ||
                  Value == "--nocompress-debug-sections") {
         CmdArgs.push_back(Value.data());
+      } else if (Value == "--crel") {
+        Crel = true;
+      } else if (Value == "--no-crel") {
+        Crel = false;
+      } else if (Value == "--allow-experimental-crel") {
+        ExperimentalCrel = true;
       } else if (Value == "-mrelax-relocations=yes" ||
                  Value == "--mrelax-relocations=yes") {
         UseRelaxRelocations = true;
@@ -2688,6 +2696,16 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
   }
   if (ImplicitIt.size())
     AddARMImplicitITArgs(Args, CmdArgs, ImplicitIt);
+  if (Crel) {
+    if (!ExperimentalCrel)
+      D.Diag(diag::err_drv_experimental_crel);
+    if (Triple.isOSBinFormatELF() && !Triple.isMIPS()) {
+      CmdArgs.push_back("--crel");
+    } else {
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << "-Wa,--crel" << D.getTargetTriple();
+    }
+  }
   if (!UseRelaxRelocations)
     CmdArgs.push_back("-mrelax-relocations=no");
   if (UseNoExecStack)
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 2a4c1369f5a73..604fb89501730 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1133,6 +1133,27 @@ void tools::addLTOOptions(const ToolChain &ToolChain, const ArgList &Args,
 
   addMachineOutlinerArgs(D, Args, CmdArgs, ToolChain.getEffectiveTriple(),
                          /*IsLTO=*/true, PluginOptPrefix);
+
+  for (const Arg *A : Args.filtered(options::OPT_Wa_COMMA)) {
+    bool Crel = false;
+    for (StringRef V : A->getValues()) {
+      if (V == "--crel")
+        Crel = true;
+      else if (V == "--no-crel")
+        Crel = false;
+      else
+        continue;
+      A->claim();
+    }
+    if (Crel) {
+      if (Triple.isOSBinFormatELF() && !Triple.isMIPS()) {
+        CmdArgs.push_back(Args.MakeArgString(Twine(PluginOptPrefix) + "-crel"));
+      } else {
+        D.Diag(diag::err_drv_unsupported_opt_for_target)
+            << "-Wa,--crel" << D.getTargetTriple();
+      }
+    }
+  }
 }
 
 /// Adds the '-lcgpu' and '-lmgpu' libraries to the compilation to include the
diff --git a/clang/test/Driver/crel.c b/clang/test/Driver/crel.c
new file mode 100644
index 0000000000000..a427799c9ab6f
--- /dev/null
+++ b/clang/test/Driver/crel.c
@@ -0,0 +1,25 @@
+// RUN: not %clang -### -c --target=x86_64 -Wa,--crel %s 2>&1 | FileCheck %s --check-prefix=NOEXP
+
+// NOEXP: error: -Wa,--allow-experimental-crel must be specified to use -Wa,--crel. CREL is experimental and takes a non-standard section type code
+
+// RUN: %clang -### -c --target=x86_64 -Wa,--crel,--allow-experimental-crel %s -Werror 2>&1 | FileCheck %s
+// RUN: %clang -### -c --target=x86_64 -Wa,--crel,--no-crel,--allow-experimental-crel %s -Werror 2>&1 | FileCheck %s --check-prefix=NO
+// RUN: %clang -### -c --target=x86_64 -Wa,--allow-experimental-crel %s -Werror 2>&1 | FileCheck %s --check-prefix=NO
+// RUN: not %clang -### -c --target=arm64-apple-darwin -Wa,--crel,--allow-experimental-crel %s 2>&1 | FileCheck %s --check-prefix=ERR
+// RUN: not %clang -### -c --target=mips64 -Wa,--crel,--allow-experimental-crel %s 2>&1 | FileCheck %s --check-prefix=ERR
+
+// RUN: %clang -### -c --target=aarch64 -Werror -Wa,--crel,--allow-experimental-crel -x assembler %s -Werror 2>&1 | FileCheck %s --check-prefix=ASM
+// RUN: not %clang -### -c --target=mips64 -Wa,--crel,--allow-experimental-crel -x assembler %s 2>&1 | FileCheck %s --check-prefix=ERR
+
+// CHECK: "-cc1" {{.*}}"--crel"
+// NO:     "-cc1"
+// NO-NOT: "--crel"
+// ASM:   "-cc1as" {{.*}}"--crel"
+// ERR: error: unsupported option '-Wa,--crel' for target '{{.*}}'
+
+/// Don't bother with --allow-experimental-crel for LTO.
+// RUN: %clang -### --target=x86_64-linux -Werror -flto -Wa,--crel %s 2>&1 | FileCheck %s --check-prefix=LTO
+// LTO:       "-plugin-opt=-crel"
+
+// RUN: touch %t.o
+// RUN: not %clang -### --target=mips64-linux-gnu -flto -Wa,--crel %t.o 2>&1 | FileCheck %s --check-prefix=ERR
diff --git a/clang/test/Misc/cc1as-crel.s b/clang/test/Misc/cc1as-crel.s
new file mode 100644
index 0000000000000..78e78b09bf4c3
--- /dev/null
+++ b/clang/test/Misc/cc1as-crel.s
@@ -0,0 +1,6 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang -cc1as -triple x86_64 %s -filetype obj --crel -o %t.o
+// RUN: llvm-readelf -S %t.o | FileCheck %s
+
+// CHECK: .crel.text CREL
+call foo
diff --git a/clang/tools/driver/cc1as_main.cpp b/clang/tools/driver/cc1as_main.cpp
index ce1e181042609..4e0aa1450563e 100644
--- a/clang/tools/driver/cc1as_main.cpp
+++ b/clang/tools/driver/cc1as_main.cpp
@@ -164,6 +164,9 @@ struct AssemblerInvocation {
   LLVM_PREFERRED_TYPE(bool)
   unsigned EmitCompactUnwindNonCanonical : 1;
 
+  LLVM_PREFERRED_TYPE(bool)
+  unsigned Crel : 1;
+
   /// The name of the relocation model to use.
   std::string RelocationModel;
 
@@ -204,6 +207,7 @@ struct AssemblerInvocation {
     EmbedBitcode = 0;
     EmitDwarfUnwind = EmitDwarfUnwindType::Default;
     EmitCompactUnwindNonCanonical = false;
+    Crel = false;
   }
 
   static bool CreateFromArgs(AssemblerInvocation &Res,
@@ -373,6 +377,7 @@ bool AssemblerInvocation::CreateFromArgs(AssemblerInvocation &Opts,
 
   Opts.EmitCompactUnwindNonCanonical =
       Args.hasArg(OPT_femit_compact_unwind_non_canonical);
+  Opts.Crel = Args.hasArg(OPT_crel);
 
   Opts.AsSecureLogFile = Args.getLastArgValue(OPT_as_secure_log_file);
 
@@ -430,6 +435,7 @@ static bool ExecuteAssemblerImpl(AssemblerInvocation &Opts,
   MCOptions.EmitDwarfUnwind = Opts.EmitDwarfUnwind;
   MCOptions.EmitCompactUnwindNonCanonical = Opts.EmitCompactUnwindNonCanonical;
   MCOptions.MCSaveTempLabels = Opts.SaveTemporaryLabels;
+  MCOptions.Crel = Opts.Crel;
   MCOptions.X86RelaxRelocations = Opts.RelaxELFRelocations;
   MCOptions.CompressDebugSections = Opts.CompressDebugSections;
   MCOptions.AsSecureLogFile = Opts.AsSecureLogFile;

jh7370

I've skimmed through this, and it looks okay, but I don't have enough experience with the clang driver to feel comfortable signing it off.

smithp35

A few small comments, but otherwise looks good to me.

smithp35 · 2024-07-02T09:32:58Z

clang/include/clang/Basic/DiagnosticDriverKinds.td

@@ -801,6 +801,9 @@ def warn_drv_missing_multilib : Warning<
 def note_drv_available_multilibs : Note<
  "available multilibs are:%0">;

+def err_drv_experimental_crel : Error<
+  "-Wa,--allow-experimental-crel must be specified to use -Wa,--crel. CREL is experimental and takes a non-standard section type code">;


Nit, I think "uses a non-standard section type code" would read better here.

Thx. Adopted.

smithp35 · 2024-07-02T09:46:05Z

clang/test/Driver/crel.c

+// ASM:   "-cc1as" {{.*}}"--crel"
+// ERR: error: unsupported option '-Wa,--crel' for target '{{.*}}'
+
+/// Don't bother with --allow-experimental-crel for LTO.


I think it would be better to say
/// --allow-experimental-crel is not required for LTO.

Could be worth mentioning that in the description.

"is unlikely to be desirable for LTO"? (crel is never required)

I picked: The --allow-experimental-crel error check doesn't apply to LTO.

smithp35 · 2024-07-02T09:49:16Z

clang/test/Driver/crel.c

+// RUN: %clang -### -c --target=x86_64 -Wa,--allow-experimental-crel %s -Werror 2>&1 | FileCheck %s --check-prefix=NO
+// RUN: not %clang -### -c --target=arm64-apple-darwin -Wa,--crel,--allow-experimental-crel %s 2>&1 | FileCheck %s --check-prefix=ERR
+// RUN: not %clang -### -c --target=mips64 -Wa,--crel,--allow-experimental-crel %s 2>&1 | FileCheck %s --check-prefix=ERR
+


Worth a test that crel isn't supported, for --no-integrated-as ?

Good idea. Added

Created using spr 1.3.5-bogner

MaskRay · 2024-07-03T16:43:06Z

If this looks good now, may I get a stamp? :)

smithp35

My apologies, I missed this one in my github mail folder.

LGTM, thanks for the update.

Created using spr 1.3.5-bogner

The two options are discussed in a few comments around llvm#91280 (comment) * -Wa,--crel: error "-Wa,--allow-experimental-crel must be specified to use -Wa,--crel..." * -Wa,--allow-experimental-crel: no-op * -Wa,--crel,--allow-experimental-crel: enable CREL in the integrated assembler (llvm#91280) MIPS's little-endian n64 ABI messed up the `r_info` field in relocations. While this could be fixed with CREL, my intention is to avoid complication in assembler/linker. The implementation simply doesn't allow CREL for MIPS. Link: https://discourse.llvm.org/t/rfc-crel-a-compact-relocation-format-for-elf/77600 Pull Request: llvm#97378

Follow-up to #97378

[𝘀𝗽𝗿] initial version

6e47264

Created using spr 1.3.5-bogner

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" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Jul 2, 2024

MaskRay requested review from dwblaikie, smithp35 and jh7370 July 2, 2024 02:45

MaskRay requested review from petrhosek and brad0 July 2, 2024 02:46

jh7370 reviewed Jul 2, 2024

View reviewed changes

smithp35 reviewed Jul 2, 2024

View reviewed changes

add -fno-integrated-as test. adjust diagnostic

56a1e99

Created using spr 1.3.5-bogner

smithp35 approved these changes Jul 3, 2024

View reviewed changes

rebase. minor test code style change

d54e12b

Created using spr 1.3.5-bogner

MaskRay merged commit 04a1a34 into main Jul 3, 2024
4 of 6 checks passed

MaskRay deleted the users/MaskRay/spr/driver-add-wa-options-crel-and-allow-experimental-crel branch July 3, 2024 20:45

MaskRay mentioned this pull request Jul 19, 2024

[AArch64,ELF] Allow implicit $d/$x at section beginning #99718

Merged

MaskRay added a commit that referenced this pull request Aug 16, 2024

[Driver] Correctly handle -Wa,--crel -Wa,--no-crel

812e049

Follow-up to #97378

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Driver] Add -Wa, options --crel and --allow-experimental-crel #97378

[Driver] Add -Wa, options --crel and --allow-experimental-crel #97378

MaskRay commented Jul 2, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 2, 2024 •

edited

Loading

Uh oh!

jh7370 left a comment

Uh oh!

smithp35 left a comment

Uh oh!

smithp35 Jul 2, 2024

Uh oh!

MaskRay Jul 2, 2024

Uh oh!

smithp35 Jul 2, 2024

Uh oh!

dwblaikie Jul 2, 2024

Uh oh!

MaskRay Jul 2, 2024

Uh oh!

smithp35 Jul 2, 2024

Uh oh!

MaskRay Jul 2, 2024

Uh oh!

MaskRay commented Jul 3, 2024

Uh oh!

smithp35 left a comment

Uh oh!

Uh oh!

Uh oh!

[Driver] Add -Wa, options --crel and --allow-experimental-crel #97378

[Driver] Add -Wa, options --crel and --allow-experimental-crel #97378

Conversation

MaskRay commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

smithp35 Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

dwblaikie Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

smithp35 Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay commented Jul 3, 2024

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MaskRay commented Jul 2, 2024 •

edited

Loading

llvmbot commented Jul 2, 2024 •

edited

Loading