[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/CMakeLists.txt:21
   ScriptParser.cpp
+  GdbIndex.cpp
   Strings.cpp
----------------
ruiu wrote:
> Sort
Done.


================
Comment at: ELF/Config.h:101-102
   bool FatalWarnings;
+  bool GdbIndex;
   bool GcSections;
   bool GnuHash = false;
----------------
ruiu wrote:
> Sort
> 
Done.


================
Comment at: ELF/GdbIndex.cpp:12
+//
+// If that option is used, linker should emit a .gdb_index section that allows
+// debugger to locate and read .dwo files, containing neccessary debug
----------------
ruiu wrote:
> This long comment doesn't match with what you do in this patch. You want to add this later.
I believe it is fine. It describes what is gdb_index and what linker should do to support this feature. 
And at the end it contains what we (in this patch) do now:

> 
> // Current version of implementation has 1, 2, 3 steps. So it writes .gdb_index
> // header and list of compilation units. Since we so not plan to support types
> // CU list area, it is also empty and so far is "implemented".
> // Other data areas are not yet implemented.

So during next patches I plan to modify that last part and finally remove it when all be done.



================
Comment at: ELF/GdbIndex.h:22
+
+template <class ELFT> class GdbIndexBuilder {
+  typedef typename ELFT::uint uintX_t;
----------------
ruiu wrote:
> You can remove this class and replace a non-member function `readCuList(InputSection<ELFT> *)`.
Ok, though I am thinking about this patch as about skeleton which intention is to reduce changes for futher parts.
Removing this class much probably means I`ll need to restore it in next one anyways.


================
Comment at: ELF/OutputSections.cpp:90
+template <class ELFT> void GdbIndexSection<ELFT>::writeTo(uint8_t *Buf) {
+  write32le(Buf, 7);                  // Write version.
+  write32le(Buf + 4, CuListOffset);   // CU list offset.
----------------
ruiu wrote:
> ruiu wrote:
> > Remove `.` from comments because they are not sentences.
> `Write version` -> `Version` for consistency.
Done.


================
Comment at: ELF/OutputSections.cpp:90-95
+  write32le(Buf, 7);                  // Write version.
+  write32le(Buf + 4, CuListOffset);   // CU list offset.
+  write32le(Buf + 8, CuTypesOffset);  // Types CU list offset.
+  write32le(Buf + 12, CuTypesOffset); // Address area offset.
+  write32le(Buf + 16, CuTypesOffset); // Symbol table offset.
+  write32le(Buf + 20, CuTypesOffset); // Constant pool offset.
----------------
grimar wrote:
> ruiu wrote:
> > ruiu wrote:
> > > Remove `.` from comments because they are not sentences.
> > `Write version` -> `Version` for consistency.
> Done.
Done.


================
Comment at: ELF/Writer.cpp:777
   Out<ELFT>::FiniArray = findSection(".fini_array");
 
+  if (Out<ELFT>::GdbIndex)
----------------
ruiu wrote:
> I think you want to do the same as the other sections here and remove `DebugInfoSec`.
> 
>   Out<ELFT>::DebugInfo = findSection(".debug_info");
Done.


https://reviews.llvm.org/D24706