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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:65
@@ +64,3 @@
+  if (!DebugInfoSec) {
+    error(".debug_info is required for building .gdb_index");
+    return false;
----------------
ruiu wrote:
> Please include a file name in the error message.
Which one ? DebugInfoSec here is an output section:

```
  if (Out<ELFT>::GdbIndex)
    Out<ELFT>::GdbIndex->DebugInfoSec = findSection(".debug_info");
```
It is used as a container of .debug_info sections to parse. I think generally it is ok that some object may not have it.
But I checked gold and found it just does not emit the .gdb_index instead of showing error if there is no .debug_info input sections at all, so I removed that error either.

================
Comment at: ELF/OutputSections.cpp:71
@@ +70,3 @@
+  for (InputSection<ELFT> *I : IS)
+    DwarfInfoReader<ELFT>(*this, I).addToGdbIndex();
+  return true;
----------------
ruiu wrote:
> Looks like this can be written in a more functional programming style like this.
> 
>   CompilationUnits = readDwarf(I);
> 
> where readDwarf is a new function that returns a new vector.
> 
> Currently, it is not clear what DwarfInfoReader would do to its arguments.
Rewrote.

================
Comment at: ELF/OutputSections.cpp:86-110
@@ +85,27 @@
+template <class ELFT> void GdbIndexSection<ELFT>::writeTo(uint8_t *Buf) {
+  // Write version.
+  write32le(Buf, 7);
+  Buf += 4;
+
+  write32le(Buf, CuListOffset);
+  Buf += 4;
+
+  // Offset of the types CU list.
+  write32le(Buf, CuTypesOffset);
+  Buf += 4;
+
+  // Offset of the address area, the same as offset of the types CU list,
+  // as we dont support types CU lists, so it is empty.
+  write32le(Buf, CuTypesOffset);
+  Buf += 4;
+
+  // Offset of the symbol table, the same as offset of the types CU list,
+  // we do not fill it yet.
+  write32le(Buf, CuTypesOffset);
+  Buf += 4;
+
+  // Offset of the constant pool, the same as offset of the types CU list,
+  // we do not fill it yet.
+  write32le(Buf, CuTypesOffset);
+  Buf += 4;
+
----------------
ruiu wrote:
> I'd save lines here.
> 
>   write32le(Buf, 7);                   // Version
>   write32le(Buf + 4, CutListOffset);   // [what is this?]
>   write32le(Buf + 8, CutListOffset);   // Offset of the address area
>   write32le(Buf + 12, CutListOffset);  // ...
>   ...
>   Buf += 24;
> 
> Or, maybe we should define a struct for the header in libObject/libSupport.
Done. I do not think struct for just a header of some output section worth that. I think we dont have any such for other output sections, and also it looks soon CuTypesOffset can be removed (as types data area is depricated), I expect to see it in version 8, do not think we want to have structs of different versions or something like that.


https://reviews.llvm.org/D24706