Page MenuHomePhabricator

Set a debugger "target" to guide DWARF choices
ClosedPublic

Authored by probinson on Mar 20 2015, 3:41 PM.

Details

Summary

Debuggers have different preferences regarding various bits of DWARF.
Invent a debugger "target" to clarify the choices, and also simplify testing.
A follow-up patch will plumb this through from Clang.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson updated this revision to Diff 22391.Mar 20 2015, 3:41 PM
probinson retitled this revision from to Set a debugger "target" to guide DWARF choices.
probinson updated this object.
probinson edited the test plan for this revision. (Show Details)
probinson added reviewers: echristo, dblaikie, aprantl.
probinson added a subscriber: Unknown Object (MLST).
aprantl edited edge metadata.Mar 21 2015, 1:21 PM

It's unfortunate that this is necessary, but it is definitely preferable over having platform-specific behavior.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
236 ↗(On Diff #22391)

This should probably be IsDebuggerLLDB() instead of IsDarwin.

probinson added inline comments.Mar 23 2015, 1:30 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
210 ↗(On Diff #22391)

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

236 ↗(On Diff #22391)

OK.

aprantl added inline comments.Mar 23 2015, 3:15 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
210 ↗(On Diff #22391)

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

ygao added a subscriber: ygao.Mar 23 2015, 6:01 PM

Do these debuggers preserve their peculiar behaviors from version to version? Do we need to identify the version of the debugger too? For example, gdb 7.2 versus gdb 7.9.

probinson updated this revision to Diff 22592.Mar 24 2015, 12:52 PM
probinson edited edge metadata.

Update per Adrian's feedback.

In D8506#145780, @ygao wrote:

Do these debuggers preserve their peculiar behaviors from version to version? Do we need to identify the version of the debugger too? For example, gdb 7.2 versus gdb 7.9.

I think mostly debuggers are pretty consistent about things from one version to the next. I haven't run into anything that would suggest knowing about debugger versions was really necessary.

aprantl added inline comments.Apr 14 2015, 7:59 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
208 ↗(On Diff #22592)

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

probinson added inline comments.Apr 20 2015, 11:51 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
208 ↗(On Diff #22592)

"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 added inline comments.Apr 20 2015, 11:54 AM
include/llvm/CodeGen/CommandFlags.h
268 ↗(On Diff #22592)

Maybe this should be "debugger-tune" instead of "target" so that it implies smaller kinds of differences. It's certainly a lot less radical than most uses of "target" within LLVM.

Agreed. Target should be reserved for something on the scale of DWARF versus PDB.

I have one more request: This should come with a guideline or at least some examples for what kind of changes constitute "tuning". Our primary goal should still be to emit standards-compliant DWARF. Or maybe the primary goal is to support existing debuggers and following the standard is second, but we probably should discuss this.

I have one more request: This should come with a guideline or at least some examples for what kind of changes constitute "tuning". Our primary goal should still be to emit standards-compliant DWARF. Or maybe the primary goal is to support existing debuggers and following the standard is second, but we probably should discuss this.

That should happen on the dev lists, and obviously I should kick it off... I think with a new topic, as the previous one had "target" in the subject and we're agreed not to use that term for what this is about.
It's also not really obvious to me where such a guideline ought to be placed. But we can debate that on-list as well.

Something odd is going on with clang right now, I'll refresh this patch tomorrow and start that discussion.

probinson updated this revision to Diff 24749.Apr 30 2015, 10:54 AM

Update to describe this as "tuning" rather than a "target."

echristo edited edge metadata.May 6 2015, 2:52 PM

Inline comments.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
211–219 ↗(On Diff #24749)

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

At least not in the backend.

234 ↗(On Diff #24749)

This should be simplified to tuneDebugForGDB().

244 ↗(On Diff #24749)

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.

test/DebugInfo/X86/dwarf-public-names.ll
7–10 ↗(On Diff #24749)

I think I'd like this split out into a different test file.

probinson added inline comments.May 6 2015, 3:53 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
211–219 ↗(On Diff #24749)

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.

224 ↗(On Diff #24749)

Should this become tuneDebugForLLDB()?

234 ↗(On Diff #24749)

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

244 ↗(On Diff #24749)

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.

test/DebugInfo/X86/dwarf-public-names.ll
7–10 ↗(On Diff #24749)

Ironically, this is one of the tests that would break if LLVM defaulted to no-tuning; the triples would need to be replaced by tuning options. And if tuning isn't premised on the triple, then the non-default triple/tuning combos wouldn't need testing, and the new RUN lines would just go away.

emaste added a subscriber: emaste.May 11 2015, 7:40 AM
emaste added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
213 ↗(On Diff #24749)

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

probinson updated this revision to Diff 27218.Jun 5 2015, 12:35 PM
probinson edited edge metadata.

Refresh and address specific review comments:

  • pubsections based on tuning for GDB (this is correct only if "no tuning" isn't an option)
  • revert TLS opcode choice to platform-based instead of tuning-based; Eric and Adrian need to work that one out, right now I'm following the most recent expressed wish of the code owner even though I agree with Adrian
  • move test to a separate file
  • setting the default tuning remains triple-based; changing the defaults for other targets can wait for follow-up patches

I did experiment with "default to no tuning." While tests basically
worked, it exposed one minor flaw in dwarf-public-names.ll (diff
included here although it really ought to be committed separately).
However, a no-tuning default would be a distinct functional change,
not backward compatible, as pointed out by David Blaikie; so I didn't
do that in the patch.

We might still want to have a more Target-based way to specify a
default tuning, rather than using the triple, but that could be a
followup patch. Note that people associated with FreeBSD and with
Hexagon have both expressed a desire to default to LLDB.

probinson updated this revision to Diff 27395.Jun 9 2015, 1:24 PM

Add long-ago promised guidance commentary to DwarfDebug.h.
I've had time to make it shorter. ;-)

I believe that FreeBSD should also default to LLDB (cf. https://wiki.freebsd.org/GdbRetirement).

Otherwise this is good from my point of view; I'll defer to Eric for his comments.

probinson updated this revision to Diff 28471.Jun 25 2015, 10:02 AM

FreeBSD now defaults to LLDB tuning.

@Eric, can we finish this off? (I'm on vacation next week.)

dblaikie accepted this revision.Jun 25 2015, 10:16 AM
dblaikie edited edge metadata.

Looks plausible. Though the command line flag should really only be used for testing, I think - you'll want another way to specify it for actual exposure through clang, etc, if you want/need to expose it.

include/llvm/CodeGen/CommandFlags.h
236 ↗(On Diff #28471)

What's the command line argument for? I mean convenience of testing LLVM, a bit, but in general we don't want to use backend options like this when actually interacting with LLVM from Clang and other frontends. This should probably end up as a part of the CU debug info metadata? Or a module-level debug info metadata flag? (ala DWARF version)

test/DebugInfo/X86/dwarf-public-names.ll
42 ↗(On Diff #28471)

what happened to these two checks in this test?

This revision is now accepted and ready to land.Jun 25 2015, 10:16 AM
probinson added inline comments.Jun 25 2015, 11:17 AM
include/llvm/CodeGen/CommandFlags.h
236 ↗(On Diff #28471)

The command line option is precisely for testing; it is not how Clang communicates to LLVM.
CommandFlags.h exists to define command-line options for the tools (llc, opt etc) only. It does not define a command-line option that is accessible to Clang.
Clang will set the TargetOptions field directly.

test/DebugInfo/X86/dwarf-public-names.ll
42 ↗(On Diff #28471)

As originally written, this check does not fail if there is actually a debug_pubnames section, because it didn't insist that the empty line was immediately following the "debug_pubnames" header line. Instead it would match the empty line at the end of the debug_pubnames dump, and pass.
I made the same change on the LINUX path just for consistency.

This test update could actually be committed independently if you'd rather, as it's fixing a bug in the test rather than actually testing something I did in the rest of the patch.

probinson added inline comments.Jun 25 2015, 12:51 PM
include/llvm/CodeGen/CommandFlags.h
236 ↗(On Diff #28471)

David pointed out in email that using a TargetOptions field, it is hard to test that Clang got it right, and might not play nicely with LTO. I'll look at doing it with module-level metadata.

test/DebugInfo/X86/dwarf-public-names.ll
42 ↗(On Diff #28471)

Committed separately as r240679.

Some inline comments.

-eric

include/llvm/CodeGen/CommandFlags.h
236 ↗(On Diff #28471)

Module level metadata is a possibility if we think it's something that we'll need to make sure stays consistent at LTO time etc. You'll need to define a way for the metadata to merge, etc. I don't necessarily like encoding the data this way, but without a later LTO codegen pass we can pass options to it's your best bet.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
222–223 ↗(On Diff #28471)

Feel free to update this comment separately.

245 ↗(On Diff #28471)

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

probinson added inline comments.Jun 26 2015, 12:16 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
222–223 ↗(On Diff #28471)

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

245 ↗(On Diff #28471)

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 updated this revision to Diff 29287.Jul 8 2015, 2:29 PM
probinson edited edge metadata.

Move the tuning parameter out of TargetOptions, it's private to DwarfDebug now.
If you don't mind I'd prefer to do the metadata thing in a separate follow-up patch.

Replied inline. Some of this probably feels a bit vague, let me know if you've got any questions :)

-eric

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
234–235 ↗(On Diff #29287)

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

234–242 ↗(On Diff #29287)

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?

237 ↗(On Diff #29287)

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

257 ↗(On Diff #29287)

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

259 ↗(On Diff #29287)

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).

lib/CodeGen/AsmPrinter/DwarfDebug.h
572–589 ↗(On Diff #29287)

This could all go with the enum.

590–592 ↗(On Diff #29287)

Possibly rename without using Debug. We're in the debug code already.

aprantl added inline comments.Jul 13 2015, 11:02 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
259 ↗(On Diff #29287)

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)

probinson marked 5 inline comments as done.Jul 13 2015, 5:24 PM

Sorry, I couldn't really work out which things you wanted directly from triples and which would be from tuning. I'm not 100% sure phab is lining up the inline comments with the right source lines. I think I got everything else you wanted. New patch coming directly.

probinson updated this revision to Diff 29632.Jul 13 2015, 5:26 PM

Updated patch. Because the TLS opcode is based on tuning now, and FreeBSD defaults to LLDB, I had to tweak one more test.

echristo accepted this revision.Jul 14 2015, 11:11 PM
echristo edited edge metadata.

I'm a bit unhappy with the TLS op tuning, but I don't have any better ideas. It's less tuning and more correctness, but...

Anyhow, an inline comment about one of your FIXMEs, otherwise LGTM.

Thanks for all of the iterations here.

-eric

test/DebugInfo/X86/debugger-tune.ll
4 ↗(On Diff #29632)

Are you using the same accelerator tables?

probinson marked an inline comment as done.Jul 15 2015, 2:14 PM
probinson added inline comments.
test/DebugInfo/X86/debugger-tune.ll
4 ↗(On Diff #29632)

No we're not. Originally the accelerator tables were still "for Darwin" not "for LLDB" but having changed that, I can use that to take care of this FIXME. Thanks!

This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.