[PATCH] Emit DWARF info for all code section in an assembly file
Oliver Stannard
oliver.stannard at arm.com
Wed Feb 26 09:53:58 PST 2014
================
Comment at: lib/MC/MCDwarf.cpp:615
@@ -596,1 +614,3 @@
+ // The 2 byte DWARF version.
+ MCOS->EmitIntValue(context.getDwarfVersion(), 2);
----------------
Eric Christopher wrote:
> EmitInt16.
EmitInt16 is a method of AsmPrinter, and we are not outputting assembly here. Every other place in this file uses EmitIntValue.
================
Comment at: lib/MC/MCParser/AsmParser.cpp:1568
@@ -1567,4 +1567,3 @@
- // If we are generating dwarf for assembly source files and the current
- // section is the initial text section then generate a .loc directive for
- // the instruction.
+ // If we are generating dwarf for the current section then generate a .loc
+ // directive for the instruction.
----------------
Eric Christopher wrote:
> "for the current section and it contains code"?
We are emitting an instruction, so the section contains code (or will do very soon).
================
Comment at: lib/MC/MCParser/AsmParser.cpp:1571
@@ -1571,3 +1570,3 @@
if (!HadError && getContext().getGenDwarfForAssembly() &&
- getContext().getGenDwarfSection() ==
- getStreamer().getCurrentSection().first) {
+ getContext().getGenDwarfSections().count(
+ getStreamer().getCurrentSection().first)) {
----------------
Eric Christopher wrote:
> Maybe just check if the section is a text section instead of looking up in the set?
Ditto
================
Comment at: lib/MC/MCParser/ELFAsmParser.cpp:485
@@ -484,5 +484,3 @@
SectionKind Kind = computeSectionKind(Flags);
- getStreamer().SwitchSection(getContext().getELFSection(SectionName, Type,
- Flags, Kind, Size,
- GroupName),
- Subsection);
+ const MCSection* ELFSection = getContext().getELFSection(
+ SectionName, Type, Flags, Kind, Size, GroupName);
----------------
Eric Christopher wrote:
> Formatting seems odd. clang-format?
Done
================
Comment at: lib/MC/MCParser/ELFAsmParser.cpp:491
@@ +490,3 @@
+ getContext().addGenDwarfSection(ELFSection);
+ if ( getContext().getDwarfVersion() <= 2
+ && getContext().getGenDwarfSections().size() > 1)
----------------
Eric Christopher wrote:
> Formatting.
Done
================
Comment at: test/MC/ARM/ldr-pseudo.s:19
@@ -18,3 +18,3 @@
ldr r0, =0x10001
-@ CHECK: ldr r0, .Ltmp0
+@ CHECK: ldr r0, .Ltmp[[TMP0:[0-9]+]]
----------------
Eric Christopher wrote:
> What's going on here?
These tests were broken by the extra symbol inserted to reference the beginning of each section, which changed anonymous symbol names.
================
Comment at: test/MC/ARM/ltorg.s:16
@@ -15,3 +15,3 @@
ldr r0, =0x10001
-@ CHECK: ldr r0, .Ltmp0
+@ CHECK: ldr r0, .Ltmp[[TMP0:[0-9+]]]
adds r0, r0, #1
----------------
Eric Christopher wrote:
> Ditto.
Ditto
================
Comment at: tools/llvm-mc/llvm-mc.cpp:156
@@ -155,1 +155,3 @@
+enum DwarfVersionNumber {
+ DWARF2 = 2,
----------------
Eric Christopher wrote:
> Let's just use a different command line option where you can set it. There's no need to make it identical to an assembler.
We don't have to make it identical to the assembler, but I don't see a good reason to have a different option to clang, either. As it is now, we behave the exact same way as clang if the option is given 0 or 1 times, and give a sensible error message if it is given more than 1 time ("llvm-mc: for the -gdwarf-3 option: may only occur zero or one times").
================
Comment at: include/llvm/MC/MCContext.h:131
@@ +130,3 @@
+ /// Symbols created for the start and end of each section, used for
+ /// generating the debug_aranges section
+ DenseMap<const MCSection*, MCSymbol*> GenDwarfSectionStartSyms;
----------------
Eric Christopher wrote:
> debug_ranges yes?
Both. Done.
================
Comment at: include/llvm/MC/MCContext.h:383
@@ +382,3 @@
+ MCSymbol *getGenDwarfSectionStartSym(const MCSection* Sec) {
+ DenseMap<const MCSection *, MCSymbol*>::iterator Sym = GenDwarfSectionStartSyms.find(Sec);
+ if (Sym != GenDwarfSectionStartSyms.end())
----------------
Eric Christopher wrote:
> 80-columns?
Done
================
Comment at: include/llvm/MC/MCContext.h:23
@@ -22,2 +22,3 @@
#include <map>
+#include <set>
#include <vector> // FIXME: Shouldn't be needed.
----------------
Eric Christopher wrote:
> SetVector here perhaps? Stable iteration is important, but I'm not sure how many we expect here. I.e. are we expecting the full pain of large -ffunction-sections C++ or hand-written code?
This set should stay small for sane use-cases. This code is only used when the input is an assembly file, there is a separate code path if we are generating native code from IR.
Done.
================
Comment at: lib/MC/MCDwarf.cpp:514
@@ -507,2 +513,3 @@
- // Create a symbol at the end of the section that we are creating the dwarf
+ std::set<const MCSection*> &Sections = context.getGenDwarfSections();
+ std::set<const MCSection*> ValidSections;
----------------
Eric Christopher wrote:
> If we're using this for ranges and aranges then we should just pull it out into a separate "finalizeSections" routine or some such.
Done
================
Comment at: lib/MC/MCDwarf.cpp:548
@@ -531,2 +547,3 @@
// Add the size of the pair of PointerSize'ed values for the address and size
// of the one default .text section we have in the table.
+ Length += 2 * AddrSize * ValidSections.size();
----------------
Eric Christopher wrote:
> Update comment.
Done
================
Comment at: lib/MC/MCDwarf.cpp:574
@@ -557,8 +573,3 @@
// Now emit the table of pairs of PointerSize'ed values for the section(s)
- // address and size, in our case just the one default .text section.
- const MCExpr *Addr = MCSymbolRefExpr::Create(
- context.getGenDwarfSectionStartSym(), MCSymbolRefExpr::VK_None, context);
- const MCExpr *Size = MakeStartMinusEndExpr(*MCOS,
- *context.getGenDwarfSectionStartSym(), *SectionEndSym, 0);
- MCOS->EmitAbsValue(Addr, AddrSize);
- MCOS->EmitAbsValue(Size, AddrSize);
+ // address and size
+ for (std::set<const MCSection*>::const_iterator sec = ValidSections.begin();
----------------
Eric Christopher wrote:
> Needs a full sentence here :)
Done
================
Comment at: lib/MC/MCDwarf.cpp:649
@@ +648,3 @@
+ // to the address range list for this compilation unit.
+ MCOS->EmitSymbolValue(LineSectionSymbol, 4);
+ } else {
----------------
Eric Christopher wrote:
> LineSectionSymbol?
Done
================
Comment at: lib/MC/MCDwarf.cpp:778
@@ +777,3 @@
+
+ const MCAsmInfo *asmInfo = context.getAsmInfo();
+ int AddrSize = asmInfo->getPointerSize();
----------------
Eric Christopher wrote:
> Use consistent variable naming please.
Done
================
Comment at: lib/MC/MCDwarf.cpp:854
@@ -753,3 +853,3 @@
// If there are no line table entries then do not emit any section contents.
if (context.getMCLineSections().empty())
----------------
Eric Christopher wrote:
> Unrelatedly this seems like it should be near the top.
Done
================
Comment at: lib/MC/MCDwarf.cpp:885
@@ -776,3 +884,3 @@
// We won't create dwarf labels for temporary symbols or symbols not in
- // the default text.
+ // a section which we are emitting dwarf for.
if (Symbol->isTemporary())
----------------
Eric Christopher wrote:
> Reword? Seems confusing as written.
Done
================
Comment at: lib/MC/MCDwarf.cpp:889
@@ -780,3 +888,3 @@
MCContext &context = MCOS->getContext();
- if (context.getGenDwarfSection() != MCOS->getCurrentSection().first)
+ if (!context.getGenDwarfSections().count(MCOS->getCurrentSection().first))
return;
----------------
Eric Christopher wrote:
> Comment please.
Done
================
Comment at: include/llvm/MC/MCContext.h:391
@@ -379,4 +390,3 @@
}
- MCSymbol *getGenDwarfSectionEndSym() { return GenDwarfSectionEndSym; }
- void setGenDwarfSectionEndSym(MCSymbol *Sym) {
- GenDwarfSectionEndSym = Sym;
+ MCSymbol *getGenDwarfSectionEndSym(const MCSection* Sec) {
+ DenseMap<const MCSection *, MCSymbol*>::iterator Sym = GenDwarfSectionEndSyms.find(Sec);
----------------
Eric Christopher wrote:
> We never check for NULL at the use. Either we should assert in the lookup routine or in the use, but we should check somehow.
Done
================
Comment at: lib/MC/MCDwarf.cpp:869
@@ -761,3 +868,3 @@
// Output the data for .debug_abbrev section.
- EmitGenDwarfAbbrev(MCOS);
+ EmitGenDwarfAbbrev(MCOS, UseRangesSection);
----------------
Eric Christopher wrote:
> Might want to make these two routines member functions rather than static and just have the variables be member variables. It should clean up a few conditionals in the patch.
Everything in this class is currently static, and there doesn't seem to be a big enough advantage to make the change.
================
Comment at: test/MC/ARM/dwarf-asm-multiple-sections.s:56
@@ +55,3 @@
+
+
+// RELOC: RELOCATION RECORDS FOR [.rel.debug_ranges]:
----------------
Eric Christopher wrote:
> Check the relocations in the info section?
Done
================
Comment at: test/MC/ARM/dwarf-asm-nonstandard-section.s:44
@@ +43,3 @@
+
+
+
----------------
Eric Christopher wrote:
> Same here.
Done
================
Comment at: test/MC/ARM/dwarf-asm-single-section.s:44
@@ +43,3 @@
+
+
+// RELOC-NOT: RELOCATION RECORDS FOR [.rel.debug_ranges]:
----------------
Eric Christopher wrote:
> Ditto.
Done
http://llvm-reviews.chandlerc.com/D2697
More information about the llvm-commits
mailing list