[PATCH] D74312: [DebugInfo]: Added preliminary support for DWARFv5 in llvm-dwp utility.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Please split this into smaller/more incremental patches - it should be possible to implement the new support for parsing DWARFv5 unit headers, signature, etc, without the new string header support. (you'd have to modify/hand-craft the dwo file assembly though, since LLVM's generated DWARFv5 will always use FORM_strx - but if you change the dwo to use FORM_str (direct strings, no indirection) it should mean you can make a small but valid test case without the need for strx support)



================
Comment at: llvm/test/tools/llvm-dwp/X86/simple-v5.test:1
+RUN: llvm-dwp %p/../Inputs/simple/notypes/a-v5.dwo %p/../Inputs/simple/notypes/b-v5.dwo -o %t
+RUN: llvm-dwarfdump -v %t | FileCheck %s
----------------
While previous testing has used checked in binary object files (& I wrote those tests... ) - it seems a lot of testing even of tools like this has moved ot checking in assembly files and assuming/relying on llvm-mc as an assembler to make tests more legible.

Could you do that in this case? Use/add assembly rather than binary files, and have the test assemble those files then run them through llvm-dwp?


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:593-600
+    auto getVersion = [&]() -> uint16_t {
+      DataExtractor InfoData(InfoSection, true, 0);
+      // Version field is located just after length field in Info section
+      // header. For DWARF32 length field is of 4 bytes.
+      uint64_t Offset = 4;
+      return InfoData.getU16(&Offset);
+    };
----------------
This is a rather strange construct - a named local lambda that's called only once. It doesn't seem appropriate.

If you especially want to reduce the scope of the extra variables, some options might include:

```
auto DwarfVersion;
{
  DataExtractor InfoData(InfoSection, true, 0);
  uint64_t Offset = 4;
  DwarfVersion = InfoData.getU16(&Offset);
}
```
This is usually only done if there's repeated use of similarly named variables, a large scope where these variables might get confused with others, etc. Which doesn't seem to be the case here.

or
```
auto DwarfVersion = [] {
  DataExtractor InfoData(InfoSection, true, 0);
  uint64_t Offset = 4;
  return InfoData.getU16(&Offset);
}();
```

The latter is usually only for when the variable being initialized is 'const'.

I think it's probably best to just write it more directly:
```
DataExtractor InfoData(InfoSection, true, 0);
uint64_t InfoVersionOffset = 4;
writeStringsAndOffsets(..., InfoData.getU16(&InfoVersionOffset));
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74312/new/

https://reviews.llvm.org/D74312