This is an archive of the discontinued LLVM Phabricator instance.

Fix bug 19437 - Only add discriminators for DWARF 4 and above.
ClosedPublic

Authored by dnovillo on Apr 17 2014, 10:56 AM.

Details

Summary

This prevents the discriminator generation pass from triggering if
the DWARF version being used in the module is prior to 4.

Note: The test case for this is in Clang, as this is only triggered

during Clang's code generation.

Diff Detail

Event Timeline

Is there any reason the AddDescriminator pass isn't part of llc/opt/etc so it can be tested in LLVM?

dnovillo updated this revision to Unknown Object (????).Apr 17 2014, 11:27 AM

Add forgotten test.

I forgot to git add this test before sending the original patch.

dnovillo updated this revision to Unknown Object (????).Apr 17 2014, 12:03 PM

Move getDwarfVersionFromModule into Dwarf.h.

Hrm. Don't know that we can make Support depend on IR any more than we already do. Chandler? Opinions?

dblaikie added inline comments.Apr 17 2014, 1:09 PM
include/llvm/Support/Dwarf.h
957

This might be worth being out of line, maybe. Not a big deal.

lib/Transforms/Utils/AddDiscriminators.cpp
163

I'd probably order this in terms of least expensive to most, which I assume is:

NoDiscriminators ||
version < 4 ||
hasDebugInfo(F)

(but I haven't looked at the implementation of hasDebugInfo to see if it's the most costly)

And rephrase the comment to match the order?

Not necessary, just a thought, struck me as odd to have the comment say "Finally, " but the version check not to be the last thing.

test/Transforms/AddDiscriminators/no-discriminators.ll
1 ↗(On Diff #8611)

You should be able to take an existing test case and just ad another RUN line with a specified dwarf version (there should be an opt flag to override the version... but it looks like that flag only works in DwarfDebug & would need to be moved/refactored in some way (possibly as a real option for opt/llc/llvm-mc/etc)) & demonstrate that the discriminators no longer appear. We have tests like this for other dwarf version-specific features already (but they're all in DWARF generation in DwarfDebug, rather than an opt pass like this).

70 ↗(On Diff #8611)

Having the check lines way down here is a bit hard to spot - reckon it'd be better to put them up the top just after the source?

What's the modification you're trying to ensure did not occur? Testing for the exact metadata node match is a bit limiting to other changes to the debug info which we prefer not to do (because it makes it hard to change debug info without having to fix a bunch of unrelated tests).

chandlerc added inline comments.Apr 17 2014, 1:11 PM
include/llvm/Support/Dwarf.h
957

As Eric was indicating, this can't be in Support. I think this should be a method on Module personally. It needs to be somewhere in the IR library at least.

dblaikie added inline comments.Apr 17 2014, 1:13 PM
include/llvm/Support/Dwarf.h
957

Yeah, a method on Module sounds pretty reasonable to me. (it's a bit special case, I wouldn't mind a generic system for retrieving module flags with defaults, but not fussed)

dnovillo updated this revision to Unknown Object (????).Apr 17 2014, 2:15 PM
  • Move getDwarfVersionFromModule to Module::getDwarfVersion.
  • Re-arrange the checks when deciding whether to add discriminators.
dnovillo updated this revision to Unknown Object (????).Apr 17 2014, 2:17 PM
  • Remove extraneous doxygen marker.
dnovillo added inline comments.Apr 17 2014, 2:19 PM
test/Transforms/AddDiscriminators/no-discriminators.ll
1 ↗(On Diff #8611)

The pass can only rely on what's in the IR. It's not affected by flags, other than the specific one that tells it not to bother. Not sure I'm following you here.

Ok, nits from the peanut gallery, but feel free to ignore me and listen to the real reviewers. ;]

/me runs away

include/llvm/IR/Module.h
606 ↗(On Diff #8613)

Go ahead and put a '\brief' on this.

607 ↗(On Diff #8613)

Any reason not to return the strongly typed enum?

dnovillo updated this revision to Unknown Object (????).Apr 17 2014, 2:47 PM
  • Use proper doxygen markup for return value.
dnovillo closed this revision.Apr 21 2014, 12:44 PM