[SFrame][Retry] Add assembler option --gsframe #165806
Conversation
… support it in the assembler. This is the final step in assembler-level sframe support for x86. With it in place, clang produces sframe-sections that successfully link with gnu-ld. LLD support is pending some discussion.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang-codegen Author: None (Sterling-Augustine) ChangesThis plumbs the option --gsframe through the various levels needed to support it in the assembler. This is the final step in assembler-level sframe support for x86. With it in place, clang produces sframe-sections that successfully link with gnu-ld. LLD support is pending some discussion. The previous PR had a bad merge, but the only comments were as below. Both done.
the option is passed on to cc1 The test for assembler behavior simply checks that an sframe section is added, and is modeled after the similar one for crel. Full diff: https://github.com/llvm/llvm-project/pull/165806.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 90e1f8d1eb5e9..99199c3a6c34c 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -120,6 +120,8 @@ CODEGENOPT(StackSizeSection , 1, 0, Benign) ///< Set when -fstack-size-section
///< Set when -femit-compact-unwind-non-canonical is enabled.
CODEGENOPT(EmitCompactUnwindNonCanonical, 1, 0, Benign)
+CODEGENOPT(EmitSFrameUnwind, 1, 0, Benign) ///< Set when -sframe is enabled.
+
///< Set when -fxray-always-emit-customevents is enabled.
CODEGENOPT(XRayAlwaysEmitCustomEvents , 1, 0, Benign)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index cb5cb888c6da7..a7274fbdeabf1 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -7729,6 +7729,11 @@ def massembler_fatal_warnings : Flag<["-"], "massembler-fatal-warnings">,
def crel : Flag<["--"], "crel">,
HelpText<"Enable CREL relocation format (ELF only)">,
MarshallingInfoFlag<CodeGenOpts<"Crel">>;
+// The leading 'g' is misleading. This is an unwind tables option, not
+// a debug option. But uses this name for gnu compatibility.
+def gsframe : Flag<["--"], "gsframe">,
+ HelpText<"Generate .sframe unwind sections (ELF only)">,
+ MarshallingInfoFlag<CodeGenOpts<"EmitSFrameUnwind">>;
def mmapsyms_implicit : Flag<["-"], "mmapsyms=implicit">,
HelpText<"Allow mapping symbol at section beginning to be implicit, "
"lowering number of mapping symbols at the expense of some "
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 3c313149ca1fc..1f7a96af015b6 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -500,6 +500,7 @@ static bool initTargetOptions(const CompilerInstance &CI,
Options.MCOptions.EmitDwarfUnwind = CodeGenOpts.getEmitDwarfUnwind();
Options.MCOptions.EmitCompactUnwindNonCanonical =
CodeGenOpts.EmitCompactUnwindNonCanonical;
+ Options.MCOptions.EmitSFrameUnwind = CodeGenOpts.EmitSFrameUnwind;
Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
Options.MCOptions.MCUseDwarfDirectory =
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 4e8f63ea49480..f40e68b5755ae 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2626,6 +2626,14 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
llvm::codegenoptions::DebugInfoConstructor,
DwarfVersion, llvm::DebuggerKind::Default);
}
+ } else if (Value == "--gsframe") {
+ if (Triple.isOSBinFormatELF() && Triple.isX86()) {
+ CmdArgs.push_back("--gsframe");
+ } else {
+ D.Diag(diag::err_drv_unsupported_opt_for_target)
+ << Value << D.getTargetTriple();
+ break;
+ }
} else if (Value.starts_with("-mcpu") || Value.starts_with("-mfpu") ||
Value.starts_with("-mhwdiv") || Value.starts_with("-march")) {
// Do nothing, we'll validate it later.
@@ -5929,6 +5937,16 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
else if (UnwindTables)
CmdArgs.push_back("-funwind-tables=1");
+ // Sframe unwind tables are independent of the other types, and only defined
+ // for these two targets.
+ if (Arg *A = Args.getLastArg(options::OPT_gsframe)) {
+ if ((Triple.isAArch64() || Triple.isX86()) && Triple.isOSBinFormatELF())
+ CmdArgs.push_back("--gsframe");
+ else
+ D.Diag(diag::err_drv_unsupported_opt_for_target)
+ << A->getOption().getName() << TripleStr;
+ }
+
// Prepare `-aux-target-cpu` and `-aux-target-feature` unless
// `--gpu-use-aux-triple-only` is specified.
if (!Args.getLastArg(options::OPT_gpu_use_aux_triple_only) &&
diff --git a/clang/test/Driver/sframe.c b/clang/test/Driver/sframe.c
new file mode 100644
index 0000000000000..6c0969e9042a8
--- /dev/null
+++ b/clang/test/Driver/sframe.c
@@ -0,0 +1,15 @@
+// RUN: %clang -### -c --target=x86_64 -Wa,--gsframe %s -Werror 2>&1 | FileCheck %s
+// CHECK: "-cc1" {{.*}}"--gsframe"
+
+// RUN: %clang -### -c --target=x86_64 %s 2>&1 | FileCheck %s --check-prefix=NO
+// NO: "-cc1"
+
+// RUN: %clang -### -c --target=x86_64 -Werror -Wa,--gsframe -x assembler %s -Werror 2>&1 | FileCheck %s --check-prefix=ASM
+// ASM: "-cc1as" {{.*}}"--gsframe"
+
+// RUN: not %clang -### -c --target=mips64 -Wa,--gsframe %s 2>&1 | FileCheck %s --check-prefix=NOTARGETC
+// NOTARGETC: error: unsupported option '--gsframe' for target '{{.*}}'
+
+// RUN: not %clang -### -c --target=mips64 -Wa,--gsframe -x assembler %s 2>&1 | FileCheck %s --check-prefix=NOTARGETASM
+// NOTARGETASM: error: unsupported option '--gsframe' for target '{{.*}}'
+
diff --git a/clang/test/Misc/cc1as-sframe.s b/clang/test/Misc/cc1as-sframe.s
new file mode 100644
index 0000000000000..9d16f0ada32ad
--- /dev/null
+++ b/clang/test/Misc/cc1as-sframe.s
@@ -0,0 +1,8 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang -cc1as -triple x86_64 %s -filetype obj --gsframe -o %t.o
+// RUN: llvm-readelf -S %t.o | FileCheck %s
+
+// CHECK: .sframe
+.cfi_startproc
+ call foo
+.cfi_endproc
diff --git a/clang/tools/driver/cc1as_main.cpp b/clang/tools/driver/cc1as_main.cpp
index 50da2f8449a22..3187a5b5c571b 100644
--- a/clang/tools/driver/cc1as_main.cpp
+++ b/clang/tools/driver/cc1as_main.cpp
@@ -163,6 +163,10 @@ struct AssemblerInvocation {
LLVM_PREFERRED_TYPE(bool)
unsigned EmitCompactUnwindNonCanonical : 1;
+ // Whether to emit sframe unwind sections.
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned EmitSFrameUnwind : 1;
+
LLVM_PREFERRED_TYPE(bool)
unsigned Crel : 1;
LLVM_PREFERRED_TYPE(bool)
@@ -388,6 +392,7 @@ bool AssemblerInvocation::CreateFromArgs(AssemblerInvocation &Opts,
Opts.EmitCompactUnwindNonCanonical =
Args.hasArg(OPT_femit_compact_unwind_non_canonical);
+ Opts.EmitSFrameUnwind = Args.hasArg(OPT_gsframe);
Opts.Crel = Args.hasArg(OPT_crel);
Opts.ImplicitMapsyms = Args.hasArg(OPT_mmapsyms_implicit);
Opts.X86RelaxRelocations = !Args.hasArg(OPT_mrelax_relocations_no);
@@ -450,6 +455,7 @@ static bool ExecuteAssemblerImpl(AssemblerInvocation &Opts,
MCOptions.MCRelaxAll = Opts.RelaxAll;
MCOptions.EmitDwarfUnwind = Opts.EmitDwarfUnwind;
MCOptions.EmitCompactUnwindNonCanonical = Opts.EmitCompactUnwindNonCanonical;
+ MCOptions.EmitSFrameUnwind = Opts.EmitSFrameUnwind;
MCOptions.MCSaveTempLabels = Opts.SaveTemporaryLabels;
MCOptions.Crel = Opts.Crel;
MCOptions.ImplicitMapSyms = Opts.ImplicitMapsyms;
|
tarunprabhu
left a comment
tarunprabhu
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Generally, the prefixes in the PR titles (which eventually become the first lines of the commit messages), reference the subprojects. Perhaps [clang] is better here? Also, what does [Retry] refer to in the title? In the description, you could provide a link to the previous PR for other reviewers for reference.
clang/test/Driver/sframe.c
Outdated
| // CHECK: "-cc1" {{.*}}"--gsframe" | ||
|
|
||
| // RUN: %clang -### -c --target=x86_64 %s 2>&1 | FileCheck %s --check-prefix=NO | ||
| // NO: "-cc1" |
There was a problem hiding this comment.
It may be better to specifically check for the absence of --gsframe here:
// RUN: %clang -### -c --target=x86_64 %s 2>&1 | FileCheck %s --check-prefix=NO-GSFRAME
// NO-GSFRAME-NOT: "--gsframe"
clang/test/Driver/sframe.c
Outdated
| @@ -0,0 +1,15 @@ | |||
| // RUN: %clang -### -c --target=x86_64 -Wa,--gsframe %s -Werror 2>&1 | FileCheck %s | |||
| // CHECK: "-cc1" {{.*}}"--gsframe" | |||
There was a problem hiding this comment.
Consider using SAME here instead of a wildcard match. For instance:
CHECK: "-cc1"
CHECK-SAME: "--gsframe"
clang/test/Driver/sframe.c
Outdated
| // NO: "-cc1" | ||
|
|
||
| // RUN: %clang -### -c --target=x86_64 -Werror -Wa,--gsframe -x assembler %s -Werror 2>&1 | FileCheck %s --check-prefix=ASM | ||
| // ASM: "-cc1as" {{.*}}"--gsframe" |
There was a problem hiding this comment.
As with the earlier test, consider using -SAME here
| // NOTARGETC: error: unsupported option '--gsframe' for target '{{.*}}' | ||
|
|
||
| // RUN: not %clang -### -c --target=mips64 -Wa,--gsframe -x assembler %s 2>&1 | FileCheck %s --check-prefix=NOTARGETASM | ||
| // NOTARGETASM: error: unsupported option '--gsframe' for target '{{.*}}' |
There was a problem hiding this comment.
Since this option is only available on x86_64 and aarch64, it would be good to add some tests for the latter as well.
There was a problem hiding this comment.
aarch64 support in the backend is incomplete. So probably better to leave testing out until the backend is ready. I will add it here when it is more mature.
clang/test/Misc/cc1as-sframe.s
Outdated
| // RUN: %clang -cc1as -triple x86_64 %s -filetype obj --gsframe -o %t.o | ||
| // RUN: llvm-readelf -S %t.o | FileCheck %s |
There was a problem hiding this comment.
Instead of two separate RUN directives, can the output of clang be piped into llvm-readelf?
| DwarfVersion, llvm::DebuggerKind::Default); | ||
| } | ||
| } else if (Value == "--gsframe") { | ||
| if (Triple.isOSBinFormatELF() && Triple.isX86()) { |
There was a problem hiding this comment.
The comment on line 5970 says that unwind tables are only defined for x86 and aarch64, but this seems to only add --gsframe for x86. Is this intentional?
There was a problem hiding this comment.
Aarch64 is incomplete in the backend, but on its way.
Cleaned up the comment and removed that clause on 5970.
|
Thanks for the review. Fixed the title. This is a retry of #165322, and added a link in the description. |
| // The leading 'g' is misleading. This is an unwind tables option, not | ||
| // a debug option. But uses this name for gnu compatibility. | ||
| def gsframe : Flag<["--"], "gsframe">, | ||
| HelpText<"Generate .sframe unwind sections (ELF only)">, |
There was a problem hiding this comment.
Since this is only on AArch64 and X86, it may be good to mention that here. This has been done for some other options too, such as print-supported-extensions. Since AArch64 has not been supported yet, it should probably just be X86.
Clang's user-facing option -Wa,--gsframe, is pending discussion as well. We clearly need to redesign the format from the ground up. |
This is the first, "And we can't have it in clang either" reaction that I have seen. Would you accept an --allow-experimental-sframe similar to what you did with --allow-experimental-crel? |
4 participants
This plumbs the option --gsframe through the various levels needed to support it in the assembler.
This is the final step in assembler-level sframe support for x86. With it in place, clang produces sframe-sections that successfully link with gnu-ld.
LLD support is pending some discussion.
The previous PR (#165322) had a bad merge, but the only comments were as below. Both done.
the option is passed on to cc1
the correct error is emitted when an unsupported platform is used
the assembler behaves as expected (even if this has been tested elsewhere, an end-to-end "integration" test would be useful)
The test for assembler behavior simply checks that an sframe section is added, and is modeled after the similar one for crel.