[PATCH] D16292: Improved Macro emission in dwarf

[Amjad Aboud via llvm-commits]

aaboud marked 4 inline comments as done.
aaboud added a comment.

Thanks David for the comments.
I will upload a new patch soon.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:565
@@ +564,3 @@
+  MCSection *Macinfo = TLOF.getDwarfMacinfoSection();
+  if (Macinfo && !Macinfo->getBeginSymbol()) {
+    MCSymbol *MacroSectionStartSym = Asm->createTempSymbol("macro");
----------------
dblaikie wrote:
> We don't support non-existence of other DWARF sections (we don't generally check for null return from TLOF.getDwarf* functions elsewhere, so far as I can see/recall) - is there any reason we need to do it in this particular case?
No specific reason.
It is just that I used an old implementation used to be in LLVM in the past that tries to emit macros.
That implementation used to check the existence of DWARF sections.
I was not sure if I should keep it or not.

Anyway, I removed the checks.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:566
@@ +565,3 @@
+  if (Macinfo && !Macinfo->getBeginSymbol()) {
+    MCSymbol *MacroSectionStartSym = Asm->createTempSymbol("macro");
+    Macinfo->setBeginSymbol(MacroSectionStartSym);
----------------
dblaikie wrote:
> I think this is the wrong API (looking at how setBeginSymbol is called elsewhere) to create the section start symbol - but in any case, again, we don't create section start symbols anywhere in the DWARF codegen, so I don't think we need to do it for Macro info either. We just rely on their existence in all other cases, it seems?
> 
> It's probably worth doing a little test of your own to build two separate objects with macro info, link them together & check that dumping/values all make sense - that the two CU's references to their macinfo actually get relocated during linking, etc. (no need to commit a test like this - we could have a test that checks that there is a relocation for the macinfo attribute, but I'm not /too/ fussed about that, it's awkward and probably annoyingly platform specific, etc)
Sorry for my lack of understanding how the section handling works.
I debugged it a little and understand what was missing, I should have initialized the macinfo section with a name for the begin label.

I will do the manual test to verify that the generated object file is accepted by the linker.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:629
@@ -626,3 +628,3 @@
     }
   }
 
----------------
dblaikie wrote:
> addSectionDelta isn't quite right - because it doesn't account for the MachO case where relocations are not used.
> 
> DwarfCompileUnit::addSectionLabel handles this case and wraps addSectionDelta.
Now, I understand what you mean. I modified the code to use "addSectionLabel".
In your first comment you mentioned "addSectionOffset" and I could not figure out how I can use it with labels.



http://reviews.llvm.org/D16292