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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/GdbIndex.cpp:63
+// symbol table and constant pool.
+// Since we so not plan to support types CU list area, it is also empty and
+// so far is "implemented". Address area is the only not implemented part.
----------------
clayborg wrote:
> Since we "do" not plan
Done.


================
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);
----------------
clayborg wrote:
> 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.
I wonder if that can be done in the following patch then ?
As far I understand that requires changing void dumpPubSection() from DebugInfo\DWARF\DWARFContext.cpp to something,
it should not be a big problem, I just suppose it is easier to do if we have at least one user of new functionality, so we can discuss the design of that change separatelly when see needs.
What do you think ? (I am ok to do take task of doing that change either before or after anyways, have no major preference here except that landing changes of this patch first makes things a bit easier to maintain).


================
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;
----------------
clayborg wrote:
> Use StringRef here instead of "const char *"?
Done.


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


================
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);
+
----------------
clayborg wrote:
> 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.
I removed this assert. readDwarf() is now called only from parseDebugSections, which is already protected by

```
  if (Finalized)
    return;
  parseDebugSections();
```

So actually it was bit excessive assert. If for some reason this method will be called when Finalized, we should catch it in testcase anyways I think.


================
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);
----------------
clayborg wrote:
> 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.
Done.


https://reviews.llvm.org/D26283