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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:15
 #include "llvm/CodeGen/AccelTable.h"
+#include "DwarfCompileUnit.h"
 #include "llvm/ADT/STLExtras.h"
----------------
labath wrote:
> JDevlieghere wrote:
> > should be on top (clang-format should do this for you IIRC)
> clang-format puts AccelTable.h on top because that's the header this cpp file implements. The reason this looks weird is because DwarfCompileUnit is in the lib folder, so I have to use the local include  path. Otherwise, this would be down below with the rest of llvm/CodeGen includes. The other files in this folder have the same style as well.
You're right, apologies for the noise!


================
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:
> JDevlieghere wrote:
> > 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 :-) 
> The last 0 is not the null terminator (we don't need it as we specify the length explicitly). But in any case, there's no upper bound on the augmentation string length. It only needs to be a multiple of four.
In that case this sounds reasonable, unless someone else has a better suggestion :-) 


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:469
+  switch (Form) {
+  case dwarf::DW_FORM_data1:
+    Size = 1;
----------------
labath wrote:
> JDevlieghere wrote:
> > 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.
> It seems it should be fairly easy to move the DWARFFormParams class and the static DWARFFormValue::getFixedByteSize function somewhere into the BinaryFormat library. Jonas, can you give me pointers to the other places which do form size calculations?
Looks like it's not as prominent as I thought, or maybe I'm not grep'ing effectively: 
 - `DIEInteger::SizeOf`
 - `DIEEntry::SizeOf`'
 - `DWARFFormValue::getFixedByteSize` 

Theres's also `DW_EH_PE_udata4`/`DW_EH_PE_sdata4` variants that could probably benefit from a similar function:
 - `getSizeForEncoding` in `MCDwarf`
 - `DWARFDataExtractor::getEncodedPointer`


Repository:
  rL LLVM

https://reviews.llvm.org/D43286