[PATCH] D33552: [ELF] - Make implementation of .gdb index to be more natural for futher paralleling.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/GdbIndex.h:31-32
+struct CompilationUnitEntry {
+  size_t CuOffset;
+  size_t CuLength;
+};
----------------
ruiu wrote:
> uint64_t?
Yes, fixed.


================
Comment at: ELF/GdbIndex.h:40
+
+struct FileGdbIndexData {
+  std::vector<AddressEntry> AddressArea;
----------------
ruiu wrote:
> There seems a better name than `FileGdbIndexData`.  This represent a chunk of data you read from a DWARF section, right?
That is goes from few debug sections of a object, so I used "File" word. I was thinking about "all data required for bulding gdb index extracted from object file".

Would you prefer something like "GdbIndexDataChunk" ?


================
Comment at: ELF/SyntheticSections.cpp:1752
 
-void GdbIndexSection::readDwarf(InputSection *Sec) {
+static std::vector<InputSection *> getInfoSections() {
+  std::vector<InputSection *> Ret;
----------------
ruiu wrote:
> getInfo -> getDebugInfo
Done.


================
Comment at: ELF/SyntheticSections.cpp:1766-1768
+  IndexData.resize(V.size());
+  for (size_t I = 0; I < V.size(); ++I)
+    readDwarf(V[I], IndexData[I]);
----------------
ruiu wrote:
> Please write in a more natural way.
> 
>   for (InputSection *Sec : V)
>     InputData.push_back(readDwarf(Sec));
Done.


================
Comment at: ELF/SyntheticSections.cpp:1788-1789
+
+      CuVectors[Sym->CuVectorIndex].push_back((uint32_t)CuId |
+                                              (NameType.Type << 24));
+    }
----------------
ruiu wrote:
> When you bitwise-or some values, you usually want to write values from MSB to LSB.
Fixed in commit of D33551 patch.


================
Comment at: ELF/SyntheticSections.cpp:1796
+
+void GdbIndexSection::readDwarf(InputSection *Sec, FileGdbIndexData& Data) {
   Expected<std::unique_ptr<object::ObjectFile>> Obj =
----------------
ruiu wrote:
> This should return FileGdbIndexData. (In general, you want to avoid mutating a object passed as a reference if writing in a more functional way is doable.)
Done.


================
Comment at: ELF/SyntheticSections.cpp:1812
 
-    CuVectors[Sym->CuVectorIndex].push_back({CuId, Pair.second});
-  }
+static size_t findCUSize(std::vector<FileGdbIndexData> &C) {
+  size_t Ret = 0;
----------------
ruiu wrote:
> This does not "find" anything. It computes. get, compute or calculate are good choices.
Ok. FWIW that confusion comes from my native language. Where I can say "find dog in the garden" or "given x = 4 * t, find x if t == 3" and that word "find" would be the same.


================
Comment at: ELF/SyntheticSections.cpp:1819
+
+static size_t findAddrAreaSize(std::vector<FileGdbIndexData> &C) {
+  size_t Ret = 0;
----------------
ruiu wrote:
> Addr -> Address
Done.


================
Comment at: ELF/SyntheticSections.h:514
   GdbHashTab SymbolTable;
-
-  // The CU vector portion of the constant pool.
-  std::vector<std::vector<std::pair<uint32_t, uint8_t>>> CuVectors;
-
-  std::vector<AddressEntry> AddressArea;
+  std::vector<FileGdbIndexData> IndexData;
 
----------------
ruiu wrote:
> `IndexData` is not a good variable name.
Renamed.


https://reviews.llvm.org/D33552