This is an archive of the discontinued LLVM Phabricator instance.

Macro Support in Dwarf emission
ClosedPublic

Authored by aaboud on Dec 14 2015, 7:38 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 42719.Dec 14 2015, 7:38 AM
aaboud retitled this revision from to Macro Support in Dwarf emission.
aaboud updated this object.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: llvm-commits.
aprantl added inline comments.Dec 14 2015, 10:51 AM
test/DebugInfo/X86/debug-macro.ll
3 ↗(On Diff #42719)

%llc_dwarf may be preferable over llc.

aaboud updated this revision to Diff 42830.Dec 15 2015, 3:53 AM
aaboud removed rL LLVM as the repository for this revision.
aaboud marked an inline comment as done.
aaboud updated this revision to Diff 42831.Dec 15 2015, 3:57 AM

Now fixed the LIT tests correctly.

Do I have commit approval for this change list?

Thanks,
Amjad

aprantl added inline comments.Dec 23 2015, 9:44 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1874 ↗(On Diff #42831)

If this violates the spec, this should be check in the debug info verifier instead of an assertion here.

1878 ↗(On Diff #42831)

Is EOS supposed to be "end of string"? Maybe NUL or null-terminator is more common.

1891 ↗(On Diff #42831)

Same here.

1912 ↗(On Diff #42831)

maybe add an assertion/unreachable for unhandled cases?

1919 ↗(On Diff #42831)

Both here and in emitMacro I'm feeling a little uneasy about all the replicated logic between .*emit and .*getSize. It's just waiting to get out of sync :-)
Is there any sensible way to fuse the emission and the size computation (either by templatizing or passing in a lambda)?

1954 ↗(On Diff #42831)

else assert maybe?

lib/CodeGen/AsmPrinter/DwarfDebug.h
435 ↗(On Diff #42831)

Ideally, these should be a separate NFC commit. Thanks for noticing!

aaboud updated this revision to Diff 43591.Dec 24 2015, 3:34 AM
aaboud marked 5 inline comments as done.

Applied changes according to Adrian comments.

  • Moved assertion to Debug Info Verifier.
  • Encapsulate Macro Node handling into a helper member function to share common code.

Thanks Adrian for the comments.
I tried to answer most of them and uploaded a new version with the changes.
Please see my response below.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1919 ↗(On Diff #43591)

I could not find an elegant and less messy way to do share code between these functions.
Also, in DIE.cpp you can see that there is a similar implementation for "EmitValue()" and "SizeOf()" where common code is not shared!

I tried at least to encapsulate the handling of the macro node array, as it is being repeated three times in the code.

Please, let me know if you have a better idea on how to share the common code above.

aprantl edited edge metadata.Jan 4 2016, 10:23 AM

Thanks for the changes!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1919 ↗(On Diff #43591)

I'll leave this up to you, but what I'm thinking is that we could define a wrapper class for the emission functions:

class AbstractAsmStreamer {
  /// Returns the byte size of the ULEB value.
  unsigned emitULEB128(unsigned Val);
  unsigned emitInt8(unsigned char Val);
  unsigned emitBytes(StringRef Data);
  ...
};

/// Streams objects using an MCAsmStreamer.
class AsmStreamEmitter : public AbstractAsmStreamer {
  AsmStreamEmitter(MCAsmStreamer ...);
};

/// Only reports the size of the streamed objects. Is there a better name?
class SizeReporter : public AbstractAsmStreamer {
  SizeReporter(MCAsmStreamer ...);
};

and have the emit* methods always return the size:

unsigned DwarfDebug::emitMacroFile(AbstractAsmStreamer &Streamer,
                                                             DIMacroFile &F, DwarfCompileUnit &U) {
  assert(F.getMacinfoType() == dwarf::DW_MACINFO_start_file);
  unsigned Size = 0;
  Size += Streamer.EmitULEB128(dwarf::DW_MACINFO_start_file);
  Size += Streamer.EmitULEB128(F.getLine());
  DIFile *File = F.getFile();
  unsigned FID =
      U.getOrCreateSourceID(File->getFilename(), File->getDirectory());
  Size += Streamer.EmitULEB128(FID);

  Size += handleMacroNodes(F.getElements(), U);

  Size += Streamer.EmitULEB128(dwarf::DW_MACINFO_end_file);
  return Size;
}

Maybe it's possible to make this interface general enough that it also can be used in DIE.cpp. If we're worried about the performance of the virtual function call we could templatize on Streamer, but it probably doesn't matter.

aaboud updated this revision to Diff 44037.Jan 5 2016, 11:54 AM
aaboud edited edge metadata.
aaboud marked an inline comment as done.

Applied Adrian comments/suggestion.
Introduced AbstractAsmStreamer class to share implementation between "Emitter" and "SizeOf" functions.

Thanks!
LGTM with the above changes addressed.

include/llvm/CodeGen/DIE.h
43 ↗(On Diff #44037)

Please move this comment into a doxygen comment for AbstractAsmStreamer that explains the purpose of the interface.

48 ↗(On Diff #44037)

Please add a note that this (currently) doesn't actually report back the size.

52 ↗(On Diff #44037)

Probably better to use override and drop the virtual here.

59 ↗(On Diff #44037)

Are the names

class AsmStreamerBase;
class EmittingAsmStreamer;
class SizeReportingAsmStreamer;

any better?

lib/CodeGen/AsmPrinter/DIE.cpp
38 ↗(On Diff #44037)

Feel free to put the one-liners into the header file, just leave the virtual destructor as an anchor.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
576 ↗(On Diff #44037)

Couldn't this just be a stack-allocated object?

1912 ↗(On Diff #44037)

Same here. Why not just a stack-allocated object?

aprantl accepted this revision.Jan 5 2016, 2:32 PM
aprantl edited edge metadata.
This revision is now accepted and ready to land.Jan 5 2016, 2:32 PM
This revision was automatically updated to reflect the committed changes.
aaboud marked 5 inline comments as done.