This is an archive of the discontinued LLVM Phabricator instance.

Move the sysroot attribute from DIModule to DICompileUnit
ClosedPublic

Authored by aprantl on Dec 19 2019, 3:59 PM.

Details

Summary

This addresses point 1 of PR44213. (https://bugs.llvm.org/show_bug.cgi?id=44213).

The DW_AT_LLVM_sysroot attribute is used for Clang module debug info, to allow LLDB to import a Clang module from source. Currently it is part of each DW_TAG_module, however, it is the same for all modules in a compile unit. It is more efficient and less ambiguous to store it once in the DW_TAG_compile_unit.

This should have no effect on DWARF consumers other than LLDB.

Diff Detail

Event Timeline

aprantl created this revision.Dec 19 2019, 3:59 PM
Herald added a project: Restricted Project. · View Herald Transcript
aprantl marked an inline comment as done.
aprantl added inline comments.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1664

@dblaikie This is a workaround for the bug I documented in https://reviews.llvm.org/D54242#inline-649516

aprantl updated this revision to Diff 234804.Dec 19 2019, 4:15 PM

Forgot to include the LLDB patch.

teemperor added inline comments.Dec 22 2019, 6:47 AM
llvm/bindings/go/llvm/dibuilder.go
120

indentation seems of?

148

indentation seems of?

llvm/include/llvm/IR/DebugInfoMetadata.h
1282

double empty line?

aprantl updated this revision to Diff 236909.Jan 8 2020, 2:36 PM

Address feedback from Raphael.

Ping @debug-info / @dblaikie for a review.

As mentioned in the revert: https://reviews.llvm.org/rGb350c666ab65b7585bc58301b03d2b46dc6b0504

Some mysterious CFI failures I haven't looked into yet, but were pretty clearly blamed on this change & went away with a revert.

Some mysterious CFI failures I haven't looked into yet, but were pretty clearly blamed on this change & went away with a revert.

Because the flag was previously not (de)serialized and now shows up in LTO builds where it was dropped before?
Well I guess I can serialize false for now without preventing this from getting fixed.

Any opinions on this patch otherwise?

Some mysterious CFI failures I haven't looked into yet, but were pretty clearly blamed on this change & went away with a revert.

Because the flag was previously not (de)serialized and now shows up in LTO builds where it was dropped before?

Maybe, but don't see how that'd turn up anything interesting in CFI behavior - again, no one's turning this flag on...

Well I guess I can serialize false for now without preventing this from getting fixed.

Any opinions on this patch otherwise?

Seems alright - but I'd like to understand it a bit better. Looks like this'll add the attribute to every (.o - so skeleton with Split DWARF, or the normal/unit with non-split) CU, where previously it was only in DIModules which were only emitted with -gmodules? What's it used for? Could it be put in the full unit, rather than the skeleton unit? Could it be only emitted when tuning for lldb?

Some mysterious CFI failures I haven't looked into yet, but were pretty clearly blamed on this change & went away with a revert.

Because the flag was previously not (de)serialized and now shows up in LTO builds where it was dropped before?

Maybe, but don't see how that'd turn up anything interesting in CFI behavior - again, no one's turning this flag on...

Well I guess I can serialize false for now without preventing this from getting fixed.

Any opinions on this patch otherwise?

Seems alright - but I'd like to understand it a bit better. Looks like this'll add the attribute to every (.o - so skeleton with Split DWARF, or the normal/unit with non-split) CU, where previously it was only in DIModules which were only emitted with -gmodules? What's it used for? Could it be put in the full unit, rather than the skeleton unit? Could it be only emitted when tuning for lldb?

Yes and yes. I'll update the patch. The attribute helps LLDB to construct a Clang compiler invocation that can be used to import Clang modules from source so you have access to the entire module (not just what was used and thus put into the debug info) in the expression evaluator.

aprantl updated this revision to Diff 238093.Jan 14 2020, 1:49 PM

Addressed feedback from @dblaikie :

  • sysroot is now an LLDB tuning setting
  • attribute moved from skeleton to .dwo unit
dblaikie added inline comments.Jan 14 2020, 2:14 PM
clang/test/CodeGen/debug-info-sysroot.c
6 ↗(On Diff #238093)

side note: debugger tuning is not in the IR at all - so it probably doesn't work during LTO without passing it to the backend compile(s)/link step, somehow (not sure if any command line syntax supports that, though - maybe -mllvm somehow). At some point I'm guessing it should be added as module level metadata.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1421

Probably might as well phrase this as bounds - size < 5 || size > 6 - then any changes can modify the upper bound rather than rephrasing this then.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
850–851

Yeah, shouldn't this use the parameter DIUnit's SysRoot? Rather than needing/using a member of DwarfDebug?

902

This looks suspicious - if SysRoot can vary per DICompileUnit, what's the e meaning of using one of those DICompileUnit's SysRoot on DwarfDebug which is cross-CU?

aprantl updated this revision to Diff 238113.Jan 14 2020, 2:49 PM
  • removed copyied&pasted sysroot member from DwarfDebug
  • address feedback in MetadataLoader

Any other comments?

dblaikie accepted this revision.Jan 16 2020, 3:38 PM

Looks good - thanks!

This revision is now accepted and ready to land.Jan 16 2020, 3:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2020, 12:57 PM