[PATCH] D26283: [ELF] - Partial support of --gdb-index command line option (Part 3).

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

See inlined comments.



================
Comment at: ELF/GdbIndex.cpp:105-118
+    DataExtractor PubNames(D, IsLE, 0);
+    uint32_t Offset = 0;
+    while (PubNames.isValidOffset(Offset)) {
+      // Skip length, version, unit offset and size.
+      Offset += 14;
+      while (Offset < D.size()) {
+        uint32_t DieRef = PubNames.getU32(&Offset);
----------------
Might be worth putting this pubnames/pubtypes parser into the llvm/lib/DebugInfo/DWARF. We don't currently have a parser for this there. The more we duplicate code that is manually parsing DWARF sections outside of the DWARF parser the more fixes we need to make if the format gets updated.


================
Comment at: ELF/SyntheticSections.cpp:1417
+// https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html
+static uint32_t hash(const char *Str) {
+  uint32_t R = 0;
----------------
Use StringRef here instead of "const char *"?


================
Comment at: ELF/SyntheticSections.cpp:1420
+  uint8_t C;
+  while ((C = *Str++) != 0) {
+    C = tolower(C);
----------------
Use the StringRef iteration here?


================
Comment at: ELF/SyntheticSections.cpp:1429
 void GdbIndexSection<ELFT>::readDwarf(InputSection<ELFT> *I) {
-  std::vector<std::pair<uintX_t, uintX_t>> CuList = readCuList(I);
+  assert(!Finalized);
+
----------------
Maybe add a:
```
if (Finalized)
  return;
```

So that this code works if asserts are not enabled? I would be great to reduce the amounts of places that use assert unconditionally in the llvm codebase.


================
Comment at: ELF/SyntheticSections.cpp:1441
+      Builder.readPubNamesAndTypes();
+  for (std::pair<StringRef, uint8_t> &Pair : NamesAndTypes) {
+    uint32_t Hash = hash(Pair.first.data());
----------------
grimar wrote:
> clayborg wrote:
> > use std::tie here with appropriate var names since you did it below?
> I am not sure what you mean, sorry. Do you meant next form ?
> 
>   for (auto& p : someInitializingFunction()) {
>     std::tie(a, b) = p;
>     // do stuff;
>   }
> 
> If so I am probably missing for what.
Never mind, I was thinking you can use std::tie in the for loop:
```
for (std::tie(Name, Type) : NamesAndTypes) {
```
but you can't.


================
Comment at: ELF/SyntheticSections.cpp:1442
+  for (std::pair<StringRef, uint8_t> &Pair : NamesAndTypes) {
+    uint32_t Hash = hash(Pair.first.data());
+    size_t Offset = StringPool.add(Pair.first);
----------------
grimar wrote:
> clayborg wrote:
> > I am guessing we can guarantee that the StringRef is NULL terminated here since the string probably originated from a string table.
> Yes, and hash() relies on that. Do you want to change hash() signature ?
Might be safer to change the hash() signature if the only way we use it is with an llvm::StringRef.


================
Comment at: ELF/SyntheticSections.cpp:1522
+    Buf += 4;
+    for (std::pair<uint32_t, uint8_t> &P : CuVec) {
+      uint32_t Index = P.first;
----------------
clayborg wrote:
> use std::tie?
Never mind, can't use std::tie in for loop


https://reviews.llvm.org/D26283