This is an archive of the discontinued LLVM Phabricator instance.

Generalize debug info / EH emission in AsmPrinter
ClosedPublic

Authored by timurrrr on Nov 19 2013, 10:32 AM.

Details

Summary

Hi David, Eric, Rafael,

Can you please review this simple patch?
See "Adding line table debug information to LLVM on Windows" for the context.

Please note that I'm very unfamiliar with the LLVM CodeGen code yet
and 12h timezones away from you.
I'd appreciate if your feedback is clear for a remote LLVM newbie, i.e. "please do this and that like this" is preferred :)

Thanks!


Timur Iskhodzhanov,
Google

Diff Detail

Event Timeline

Adding Dave to the reviewers as I'm sure he'll have comments as well.

rnk added a comment.Nov 19 2013, 1:31 PM

Seems fine, but let Dave or Eric comment.

lib/CodeGen/AsmPrinter/DebugInfoEmitter.h
25 ↗(On Diff #5662)

Is this the right name to abstract over codeview line tables, various EH impls, and general debug info? IMO it deserves a doc comment to explain what kind of thing might implement one of these.

timurrrr updated this revision to Unknown Object (????).Nov 20 2013, 3:52 AM

Somewhat addressed Reid's comment

timurrrr added inline comments.Nov 20 2013, 3:56 AM
lib/CodeGen/AsmPrinter/DebugInfoEmitter.h
25 ↗(On Diff #5662)

Is this the right name to abstract over codeview line tables, various EH impls, and general debug info?

Agree, good point.
Initially I wanted to use this interface for DI only, but it turned out I can use it for EH as well... but forgotten to update the name.

I couldn't find a good name though. AsmPrinterObserver? DebugEHInfoEmitter? Any other ideas?

IMO it deserves a doc comment to explain what kind of thing might implement one of these.

Agree, done.

David, Eric,

Any suggestions?

echristo added inline comments.Nov 25 2013, 12:59 PM
lib/CodeGen/AsmPrinter/ARMException.cpp
57

Renames like this should probably happen separately.

lib/CodeGen/AsmPrinter/DIE.cpp
389

This is what I worry about from merging the EH and Debug Info classes. This assert should never be able to fire, and while "it's just an assert" theoretically we need that assert in a lot more places.

I guess what I'm saying is that it's not obvious the assert is necessary to me. How did you run into it in the first place?

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
87

Ditto with the renaming here.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1832

Should be able to get rid of the MachineFunction * argument here yes?

timurrrr updated this revision to Unknown Object (????).Nov 26 2013, 6:03 AM

Addressed Eric's comments

lib/CodeGen/AsmPrinter/ARMException.cpp
57

OK, committed as r195763.

lib/CodeGen/AsmPrinter/DIE.cpp
389

This code is/was the only user of getDwarfDebug() method of AP.
I'm not at all familiar with DIEEntry, but looks like it just needs to know the DWARF version being used.

It looks like before it relied on DD to exist when getRefAddrSize executes.
Frankly, I'd put something like

assert(AP->getDwarfDebug() != 0);

in this function regardless of my change.
It wasn't strictly necessary though as NULL deref would crash anyways.

That said, I believe merging EH and Debug Info has little to do with this.

I put the assert in just because I think calling getRefAddrSize() at the wrong time may silently lead to miracles.

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
87

Done

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1832

Good point.
I also updated a couple of more functions taking MF* as their args.

timurrrr updated this revision to Unknown Object (????).Nov 26 2013, 6:15 AM

Made one more step forward and got rid of the unneeded parameter for endInstruction().

timurrrr updated this revision to Unknown Object (????).Nov 26 2013, 6:17 AM

Fixed a minor formatting issue.

timurrrr updated this revision to Unknown Object (????).Nov 26 2013, 6:40 AM

... and initialize CurMI in the constructor.

Post holiday ping

echristo accepted this revision.Dec 2 2013, 11:20 AM

LGTM with the changes I've mentioned.

include/llvm/CodeGen/AsmPrinter.h
25

Not a huge fan of observer... maybe worker? Handler? I think I like Handler the most, but naming is hard so if you don't think it sounds better then go ahead :)

lib/CodeGen/AsmPrinter/AsmPrinterObserver.h
19

I think you want Support/DataTypes.h here?

lib/CodeGen/AsmPrinter/DIE.cpp
390

Let's make this AP->getDwarfDebug() != NULL as you mention above.

timurrrr added inline comments.Dec 2 2013, 11:22 AM
lib/CodeGen/AsmPrinter/DIE.cpp
390

The reason why I've changed that to getDwarfVersion() is that I removed getDwarfDebug entirely.
Do you think I should get it back?

rnk added inline comments.Dec 2 2013, 11:36 AM
lib/CodeGen/AsmPrinter/AsmPrinterObserver.h
19

Fun fact: stdint.h is available since MSVC 2010, so we can use it freely now. Unfortunately DataTypes.h defines other types as well. Someone should try untangling it though.

Committed as r196270, thanks for the review!

include/llvm/CodeGen/AsmPrinter.h
25

OK, Handler it is.

lib/CodeGen/AsmPrinter/AsmPrinterObserver.h
19

OK, done

lib/CodeGen/AsmPrinter/DIE.cpp
390

OK, brought getDwarfDebug() back and removed getDwarfVersion().

timurrrr updated this revision to Unknown Object (????).Dec 3 2013, 7:05 AM

The change got reverted, due to beginFunction() not called by one of the subclasses of AsmPrinter.

lib/Target/R600/AMDGPUAsmPrinter.cpp

55 /// We need to override this function so we can avoid
56 /// the call to EmitFunctionHeader(), which the MCPureStreamer can't handle.
57 bool AMDGPUAsmPrinter::runOnMachineFunction(MachineFunction &MF) {

Attached is a patch that addresses that.

Since the fix is trivial enough (and resembles one of the intermediate patches), I'm going to land this.

timurrrr closed this revision.Dec 3 2013, 7:19 AM

Hopefully r196288 sticks.