[PATCH] D53261: [BPF] Add BTF generation for BPF target

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D53261#1269241, @aprantl wrote:

> ONe thing we discussed in person at the LLVM dev meeting was that it might be nicer to go directly from LLVM debug info metadata -> BTF rather than go from metadata -> DWARF -> BTF.


Yeah, looking at the amount of code here, seems like it might be worth considering metadata->BTF, avoiding more reliance on the DWARF emission code (would worry that changes to DWARF emission structure, forms, etc, would break this in inconvenient ways)

Is this a feature already implemented in another compiler? How much room is there for design input? (wondering if, rather than reinventing the format, there could be an extension attribute in the DWARF saying "yeah, I guarantee I've met the requirements to use this in the driver" (or whatever it is))



================
Comment at: include/llvm/MC/MCBTFContext.h:53
+/// The .BTF section header definition
+struct BTFHeader {
+  uint16_t Magic;  ///< Magic value
----------------
Doesn't look like this type is used, except for sizeof()? Perhaps it'd be better to hardcode the size? (for example, as-is, this might not portably provide a consistent size - different implementations might use different padding, etc)


================
Comment at: include/llvm/MC/MCBTFContext.h:193
+  size_t Id; ///< type index in the BTF list, started from 1
+  struct BTFType BTFType;
+
----------------
Naming a variable the same as a type is pretty confusing - (any place where you have to qualify a type name with "struct"/"class"/"enum" is a bit of a hint that something's a bit odd) best avoided?


================
Comment at: include/llvm/MC/MCBTFContext.h:201
+
+  virtual size_t getSize() { return sizeof(struct BTFType); }
+  virtual void print(raw_ostream &OS, MCBTFContext &BTFContext);
----------------
Similarly - little hesitant to rely on the sizeof a type that's not "packed", and even that's not super awesome (non-portable, etc).


================
Comment at: lib/CodeGen/AsmPrinter/Dwarf2BTF.cpp:18
+
+Die2BTFEntry::~Die2BTFEntry() {}
+
----------------
Maybe use "= default" here to define a default dtor?


================
Comment at: lib/CodeGen/AsmPrinter/Dwarf2BTF.h:28
+/// a corresponding Die2BTF entry unless that Die is skipped.
+class Die2BTFEntry {
+protected:
----------------
Not sure LLVM really uses the '2'/too thing in its naming convention? Maybe "BTFFromDwarf" (for the file name), "BTFEntryFromDie", etc?


Repository:
  rL LLVM

https://reviews.llvm.org/D53261