[PATCH] D53736: [BTF] Add BTF DebugInfo

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/BinaryFormat/BTF.h:46
+
+enum { MAGIC = 0xeB9F, VERSION = 1 };
+
----------------
These names don't follow the LLVM coding guidelines.
Perhaps these enums should have underlying types?
`enum : uint32_t` or something?


================
Comment at: include/llvm/MC/MCObjectFileInfo.h:210
 
+  /// @{
+  MCSection *BTFSection;
----------------
/// BTF-specific sections. (BTF is ...)


================
Comment at: include/llvm/MC/MCObjectFileInfo.h:380
 
+  // BTF specific sections.
+  MCSection *getBTFSection() const { return BTFSection; }
----------------
same as above


================
Comment at: lib/CodeGen/AsmPrinter/BTFDebug.cpp:207
+void BTFTypeStruct::completeData(BTFDebug &BDebug) {
+  BTFType.NameOff = BDebug.addString(STy->getName());
+
----------------
There are suspiciously few comments in most of these functions. Is what they are doing really obvious?


================
Comment at: lib/CodeGen/AsmPrinter/BTFDebug.cpp:419
+
+void BTFDebug::visitTypeEntry(const DIType *Ty) {
+  if (!Ty || DIToIdMap.find(Ty) != DIToIdMap.end())
----------------
This function looks like it would fit better in DebugInfoFinder?http://llvm.org/doxygen/classllvm_1_1DebugInfoFinder.html

I would also check how Verifier.cpp does this and whether it could be factored out.


================
Comment at: lib/CodeGen/AsmPrinter/BTFDebug.h:119
+  }
+  void completeData(BTFDebug &BDebug);
+  void emitData(MCStreamer &OS);
----------------
please add doxygen comments to all member functions that aren't obvious getter/setters and such.


================
Comment at: lib/CodeGen/AsmPrinter/BTFDebug.h:150
+  std::vector<std::string> &getTable() { return Table; }
+  size_t addString(std::string S) {
+    // Check whether the string already exists.
----------------
This is a little long for an inline definition, perhaps move to .cpp file?


================
Comment at: lib/CodeGen/AsmPrinter/BTFDebug.h:150
+  std::vector<std::string> &getTable() { return Table; }
+  size_t addString(std::string S) {
+    // Check whether the string already exists.
----------------
aprantl wrote:
> This is a little long for an inline definition, perhaps move to .cpp file?
doxygen comment?


Repository:
  rL LLVM

https://reviews.llvm.org/D53736