This is an archive of the discontinued LLVM Phabricator instance.

[clang][Toolchains][Gnu] pass -g through to assembler
ClosedPublic

Authored by nickdesaulniers on Oct 19 2022, 5:43 PM.

Details

Summary

We've been working around this for a long time in the Linux kernel; we
bend over backwards to continue to support CC=clang (w/
-fno-integrated-as) for architectures where clang can't yet be used to
assemble the kernel's assembler sources. Supporting debug info for the
combination of CC=clang w/ GNU binutils as "GAS" has been painful.

Fix this in clang so that we can work towards dropping complexity in the
Linux kernel's build system, Kbuild, for supporting this combination of
tools.

GAS added support for -g in 2004 2.16 release via
commit 329e276daf98 ("Add support for a -g switch to GAS")

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2022, 5:43 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
nickdesaulniers requested review of this revision.Oct 19 2022, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2022, 5:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm probably going to do something similar for -gdwarf-N as a follow up.

MaskRay accepted this revision.Oct 19 2022, 6:24 PM

LGTM.

clang/lib/Driver/ToolChains/Gnu.cpp
973

Use Args.AddLastArg (see Clang.cpp)

clang/test/Driver/as-options.s
120

-g can be tested along with other pass-through options. This way we test the relative order (though usually it doesn't matter but it improves the check) and decreases the number of clang invocations.

This revision is now accepted and ready to land.Oct 19 2022, 6:24 PM
probinson added inline comments.
clang/test/Driver/as-options.s
121

Please use an explicit triple here. I believe not all targets support -fno-integrated-as.

nickdesaulniers marked 2 inline comments as done.
  • add triple, use Arg::AddLastArg

@MaskRay do you think I should make it so that -g -g0 disables passing through -g?

clang/test/Driver/as-options.s
120

Is the suggestion to:

  1. combine this test with a pre-existing RUN line? or
  2. Test all of the pass through options in this run line.

If 1, then I'm not sure which test best to combine this with.

@MaskRay do you think I should make it so that -g -g0 disables passing through -g?

Looks like this pattern can be used:

if (const Arg *A = Args.getLastArg(options::OPT_g_Group)) {
  if (!A->getOption().matches(options::OPT_g0))

Many -g options unfortunately enable/disable debug info emission: -g, -g[0123], -ggdb[0123], etc. The full rules are very complex. I think it makes sense to support a subset which is mostly likely used, i.e. -g, -g[0123]. So you may check OPT_g_Group and possibly reuse DebugLevelToInfoKind (if you want to support -ggdb0) or just hard code OPT_g0 if you just want to support -g0.

handle -g0 positionally

MaskRay added inline comments.Oct 20 2022, 10:28 AM
clang/test/Driver/as-options.s
120

If there are many independent pass-through options and one invocation suffices to test them all, you may try that. But I'll not insist on that if you think separate invocations is clearer.

If you add more debug info tests, consider whether other files may be more suitable (e.g. debug-options.c, clang-g-opts.c). For now I think as-options.s is fine.

(Note that assembler synthesized debug info for assembly files is a very minor feature. I don't know why the Linux kernel is so fond of it..).

(Note that assembler synthesized debug info for assembly files is a very minor feature. I don't know why the Linux kernel is so fond of it..).

This is _required_ for symbolicating symbols defined in assembler for stack traces. The primary use is for system wide profiling. See also b/249023842 and b/236733195.

Hmm... looks like this regresses 3db3a3b0d5c56dc7da843236e244307c4ac64860; I wonder what PR it was referring to?

  • delete test from clang/test/Driver/gcc_forward.c

Looks like binutils-gdb commit 329e276daf98fb4c8b3770a6c2f02fd22472a638 ("Add support for a -g switch to GAS") added support for -g. (I think the 2.15 release)

But that was in 2004; llvm commit 3db3a3b0d5c56dc7da843236e244307c4ac64860 was in 2016...

nickdesaulniers edited the summary of this revision. (Show Details)Oct 20 2022, 11:16 AM

Updating the description to note support for -g in gas looks like 2.16 (not 2.15) based on:

$ git describe --contains  329e276daf98  --match 'bin*'
binutils-2_16-branchpoint~2248
nickdesaulniers edited the summary of this revision. (Show Details)Oct 24 2022, 12:20 PM
This revision was landed with ongoing or failed builds.Oct 24 2022, 12:31 PM
This revision was automatically updated to reflect the committed changes.