Page MenuHomePhabricator
Differential D8506 Diff 27395 lib/CodeGen/AsmPrinter/DwarfDebug.cpp

Changeset View

Standalone View

View Options

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

Show First 20 Lines • Show All 191 Lines • ▼ Show 20 Linesstatic LLVM_CONSTEXPR DwarfAccelTable::Atom TypeAtoms[] = {
DwarfAccelTable::Atom(dwarf::DW_ATOM_type_flags, dwarf::DW_FORM_data1)}; DwarfAccelTable::Atom(dwarf::DW_ATOM_type_flags, dwarf::DW_FORM_data1)};
DwarfDebug::DwarfDebug(AsmPrinter *A, Module *M) DwarfDebug::DwarfDebug(AsmPrinter *A, Module *M)
: Asm(A), MMI(Asm->MMI), DebugLocs(A->OutStreamer->isVerboseAsm()), : Asm(A), MMI(Asm->MMI), DebugLocs(A->OutStreamer->isVerboseAsm()),
PrevLabel(nullptr), InfoHolder(A, "info_string", DIEValueAllocator), PrevLabel(nullptr), InfoHolder(A, "info_string", DIEValueAllocator),
UsedNonDefaultText(false), UsedNonDefaultText(false),
SkeletonHolder(A, "skel_string", DIEValueAllocator), SkeletonHolder(A, "skel_string", DIEValueAllocator),
IsDarwin(Triple(A->getTargetTriple()).isOSDarwin()), IsDarwin(Triple(A->getTargetTriple()).isOSDarwin()),
IsPS4(Triple(A->getTargetTriple()).isPS4()),
AccelNames(DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset, AccelNames(DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset,
dwarf::DW_FORM_data4)), dwarf::DW_FORM_data4)),
AccelObjC(DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset, AccelObjC(DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset,
dwarf::DW_FORM_data4)), dwarf::DW_FORM_data4)),
AccelNamespace(DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset, AccelNamespace(DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset,
dwarf::DW_FORM_data4)), dwarf::DW_FORM_data4)),
AccelTypes(TypeAtoms) { AccelTypes(TypeAtoms) {
CurFn = nullptr; CurFn = nullptr;
CurMI = nullptr; CurMI = nullptr;
// Make sure we know our "debugger tuning."
aprantlUnsubmitted
Not Done
ReplyInline Actions

Should we rename DebuggerKind::Any to TargetDefault or just Default?

aprantl: Should we rename DebuggerKind::Any to TargetDefault or just Default?
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

"Default" works for me. I originally had some vague notion about a generic target, so "Any," but getting that to work right is surely more trouble than it's worth. "Default" better expresses that it will actually turn into something specific.

probinson: "Default" works for me. I originally had some vague notion about a generic target, so "Any,"…
bool IsPS4 = Triple(Asm->getTargetTriple()).isPS4CPU();
if (Asm->TM.Options.DebuggerTuning == DebuggerKind::Default) {
emasteUnsubmitted
Not Done
ReplyInline Actions

Note that on FreeBSD as on Darwin we set -fstandalone-debug by default.

emaste: Note that on FreeBSD as on Darwin we set `-fstandalone-debug` by default.
if (IsDarwin)
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

Should this set LLDB for Darwin? I wasn't sure.

probinson: Should this set LLDB for Darwin? I wasn't sure.
aprantlUnsubmitted
Not Done
ReplyInline Actions

Yes, recent versions of Darwin ship with LLDB as the system debugger.

aprantl: Yes, recent versions of Darwin ship with LLDB as the system debugger.
Asm->TM.Options.DebuggerTuning = DebuggerKind::LLDB;
else if (IsPS4)
Asm->TM.Options.DebuggerTuning = DebuggerKind::SCE;
else
Asm->TM.Options.DebuggerTuning = DebuggerKind::GDB;
echristoUnsubmitted
Not Done
ReplyInline Actions

I might have lost this part of the discussion but I thought the default was "no tuning"?

At least not in the backend.

echristo: I might have lost this part of the discussion but I thought the default was "no tuning"? At…
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

That did come up, and it was not clear what it should mean, so I fell back on preserving the prior behavior, which effectively defaults to GDB except for Darwin and PS4.

So, what would no-tuning mean? "No non-standard things" seems reasonably clear, but should it include all the optional-but-standard things that LLVM knows how to do? Which I guess would be pubnames + aranges + type units (are there more?).
It would smoke out all the tests that have been relying on the triple to get the right effect, though, which could be viewed as a Good Thing.

Presumably you'd want Clang to tell LLVM what tuning to use, though? Having Clang default to "no tuning" seems user-hostile.

probinson: That did come up, and it was not clear what it should mean, so I fell back on preserving the…
}
// Turn on accelerator tables for Darwin by default, pubnames by // Turn on accelerator tables for Darwin by default, pubnames by
// default for non-Darwin/PS4, and handle split dwarf. // default for GDB, and handle split dwarf.
echristoUnsubmitted
Not Done
ReplyInline Actions

Feel free to update this comment separately.

echristo: Feel free to update this comment separately.
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

Hm? The comment update reflects the code change for setting HasDwarfPubSections (line 235). Did you have something else in mind?

probinson: Hm? The comment update reflects the code change for setting HasDwarfPubSections (line 235).
echristoUnsubmitted
Not Done
ReplyInline Actions

Yeah, anything you're changing under the default (or triple) mode should go into a separate patch I'd think?

echristo: Yeah, anything you're changing under the default (or triple) mode should go into a separate…
if (DwarfAccelTables == Default) if (DwarfAccelTables == Default)
HasDwarfAccelTables = IsDarwin; HasDwarfAccelTables = IsDarwin;
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

Should this become tuneDebugForLLDB()?

probinson: Should this become tuneDebugForLLDB()?
echristoUnsubmitted
Not Done
ReplyInline Actions

For now yes. Post-dwarf5 it'll be more interesting.

echristo: For now yes. Post-dwarf5 it'll be more interesting.
else else
HasDwarfAccelTables = DwarfAccelTables == Enable; HasDwarfAccelTables = DwarfAccelTables == Enable;
if (SplitDwarf == Default) if (SplitDwarf == Default)
HasSplitDwarf = false; HasSplitDwarf = false;
echristoUnsubmitted
Not Done
ReplyInline Actions

I've actually come around to thoughts of a triple based set of defaults. I.e. go ahead and use the triple to default behaviors like this and use the tuning parameter otherwise. i.e. feature based on the default debugger for the platform. It'll involve splitting this patch up a bit (see previous comment) but that seems reasonable yes?

echristo: I've actually come around to thoughts of a triple based set of defaults. I.e. go ahead and use…
else else
HasSplitDwarf = SplitDwarf == Enable; HasSplitDwarf = SplitDwarf == Enable;
if (DwarfPubSections == Default) if (DwarfPubSections == Default)
HasDwarfPubSections = !IsDarwin && !IsPS4; HasDwarfPubSections = tuneDebugForGDB();
echristoUnsubmitted
Not Done
ReplyInline Actions

This should be simplified to tuneDebugForGDB().

echristo: This should be simplified to tuneDebugForGDB().
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

Okay. Not being Darwin-aware or an LLDB user, I was a bit reluctant to convert all the Darwin decision-making right away.

probinson: Okay. Not being Darwin-aware or an LLDB user, I was a bit reluctant to convert all the Darwin…
else else
HasDwarfPubSections = DwarfPubSections == Enable; HasDwarfPubSections = DwarfPubSections == Enable;
unsigned DwarfVersionNumber = Asm->TM.Options.MCOptions.DwarfVersion; unsigned DwarfVersionNumber = Asm->TM.Options.MCOptions.DwarfVersion;
DwarfVersion = DwarfVersionNumber ? DwarfVersionNumber DwarfVersion = DwarfVersionNumber ? DwarfVersionNumber
: MMI->getModule()->getDwarfVersion(); : MMI->getModule()->getDwarfVersion();
// Darwin and PS4 use the standard TLS opcode (defined in DWARF 3). // Darwin and PS4 use the standard TLS opcode (defined in DWARF 3).
// Everybody else uses GNU's. // Everybody else uses GNU's.
UseGNUTLSOpcode = !(IsDarwin || IsPS4) || DwarfVersion < 3; UseGNUTLSOpcode = !(IsDarwin || IsPS4) || DwarfVersion < 3;
aprantlUnsubmitted
Not Done
ReplyInline Actions

This should probably be IsDebuggerLLDB() instead of IsDarwin.

aprantl: This should probably be IsDebuggerLLDB() instead of IsDarwin.
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

OK.

probinson: OK.
echristoUnsubmitted
Not Done
ReplyInline Actions

I don't like the use of TT.isPS4CPU() here on first look, can you explain a bit?

echristo: I don't like the use of TT.isPS4CPU() here on first look, can you explain a bit?
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

IsPS4 had been a bool field in the class, but as it was never used outside the constructor that seemed unnecessary so I removed it.
I capture the triple once (line 212) as it's not all that cheap, and lets me decide about PS4 as well as FreeBSD (see lines 214 and 216).
I could capture IsPS4 as a local bool if you'd prefer, as I do make that test twice, but isPS4CPU() is relatively cheap so it didn't seem worthwhile.

probinson: IsPS4 had been a bool field in the class, but as it was never used outside the constructor that…
echristoUnsubmitted
Done
ReplyInline Actions

Yeah. I agree with Adrian here. Let's use "isDebuggerLLDB()".

echristo: Yeah. I agree with Adrian here. Let's use "isDebuggerLLDB()".
Asm->OutStreamer->getContext().setDwarfVersion(DwarfVersion); Asm->OutStreamer->getContext().setDwarfVersion(DwarfVersion);
echristoUnsubmitted
Not Done
ReplyInline Actions

I don't think this is a tuning option, and that's the problem. This is a correctness issue in that it just won't work rather than being inefficient or superfluous.

echristo: I don't think this is a tuning option, and that's the problem. This is a correctness issue in…
probinsonAuthorUnsubmitted
Not Done
ReplyInline Actions

Yeah, trying to work around a GDB bug. GDB can't tolerate the standard opcode; SCE can't tolerate the GNU opcode; I don't know what LLDB thinks, but Adrian liked having the standard opcode.
Do you have an alternate suggestion? Leaving it target-based seems worse, except for the historical consistency.

probinson: Yeah, trying to work around a GDB bug. GDB can't tolerate the standard opcode; SCE can't…
echristoUnsubmitted
Done
ReplyInline Actions

Yeah, let's put it with tuning with a what you just said as a comment (if you have a gdb bug to point at that'd be good).

echristo: Yeah, let's put it with tuning with a what you just said as a comment (if you have a gdb bug to…
aprantlUnsubmitted
Done
ReplyInline Actions

https://llvm.org/bugs/show_bug.cgi?id=18423
points to
Bug 11616 - DW_OP_form_tls_address is unimplemented
(https://sourceware.org/bugzilla/show_bug.cgi?id=11616)

aprantl: https://llvm.org/bugs/show_bug.cgi?id=18423 points to Bug 11616 - DW_OP_form_tls_address is…
{ {
NamedRegionTimer T(DbgTimerName, DWARFGroupName, TimePassesIsEnabled); NamedRegionTimer T(DbgTimerName, DWARFGroupName, TimePassesIsEnabled);
beginModule(); beginModule();
} }
} }
// Define out of line so we don't have to include DwarfUnit.h in DwarfDebug.h. // Define out of line so we don't have to include DwarfUnit.h in DwarfDebug.h.
▲ Show 20 Lines • Show All 1,754 Lines • Show Last 20 Lines