[PATCH] D43286: [CodeGen] Generate DWARF v5 Accelerator Tables

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 7 09:10:51 PST 2018


JDevlieghere added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:193
+    uint32_t AugmentationStringSize = sizeof(AugmentationString);
+    char AugmentationString[8] = {'L', 'L', 'V', 'M', '0', '7', '0', '0'};
+    static_assert(sizeof(AugmentationString) % 4 == 0, "");
----------------
labath wrote:
> aprantl wrote:
> > JDevlieghere wrote:
> > > Hmm, we should probably move this elsewhere and have Cmake generate it for us? This seems hard to maintain and quite a burden, wouldn't you agree?
> > What does the '0700' derive from?
> If we always want to put the current version here, then that would definitely be a good thing to do. However, it wasn't clear to me whether we want to do that, or only bump the version when we make a substantial change in the information we generate (I hardcoded it because I assumed the latter). However, maybe we shouldn't even follow the llvm versions, but put some completely independent strings here ? I'm also starting to think the format should be more human-readable (e.g. "LLVM 7.0.0" instead of "LLVM0700").
> 
> Basically, I have no idea what would be a good thing to put here, because I don't know how (or if) will we end up using it. I am open to any suggestions...
If we only want to change this when making changes to the format we'd probably want to use something other than the compiler version? Also, I'm guessing the last 0 is the null terminator, so that means we cant' change it for minor versions (which probably we shouldn't anyway, but who knows). Counting from zero/one also seems weird. 

Anyway I think it's good to have this, just not sure what to put there. I'll sleep on it and get back to you tomorrow :-) 


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:233
+                        const DwarfDebug &DD,
+                        ArrayRef<std::unique_ptr<DwarfCompileUnit>> CompUnits);
+
----------------
labath wrote:
> JDevlieghere wrote:
> > It seems odd that we sink the DwarfCompileUnits, why's that? 
> What do you mean by "sink" ? I am not taking the ownership of the compile units.. I couldn't even do that as the (non-mutable) ArrayRef prevents me from modifying the unique_ptrs in the list.
Right, I missed the "Ref" part in ArrayRef!


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:469
+  switch (Form) {
+  case dwarf::DW_FORM_data1:
+    Size = 1;
----------------
labath wrote:
> aprantl wrote:
> > Don't we have this somewhere else already and if not, should it be factored into a common utility function?
> We have something in the DebugInfo library, but I haven't seen anything in CodeGen. I'll have another look tomorrow...
I've seen similar code quite a bit the last few weeks but always in different parts which made it hard to reuse. Maybe it's time to put something as general as possible in Dwarf.def? The problem is usually that there's no fixed length for (U)LEB128 encoded forms.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:297
+  } else
+    AccelTableKind = DwarfAccelTables;
 
----------------
labath wrote:
> JDevlieghere wrote:
> > This doesn't seem right, if you specify apple explicitly you'll end up with DWARF? Shouldn't we just omit the else?
> You'll end up with whatever was you specified on the command line. DwarfAccelTables is the cl::opt option. Maybe I should rename it to something else to make it more clear.
Yeah, I think that'd be less confusing. 


Repository:
  rL LLVM

https://reviews.llvm.org/D43286





More information about the llvm-commits mailing list