This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on May 25 2017, 9:12 AM.

Diff Detail

Event Timeline

grimar created this revision.May 25 2017, 9:12 AM
ruiu edited edge metadata.May 25 2017, 3:43 PM

Overall, this patch is a bit too scarce of comments. You want to keep it readable for those who don't know what the code is intended to do.

ELF/GdbIndex.h
54–55

uint64_t?

63

There seems a better name than FileGdbIndexData. This represent a chunk of data you read from a DWARF section, right?

ELF/SyntheticSections.cpp
1758–1800

getInfo -> getDebugInfo

1772–1774

Please write in a more natural way.

for (InputSection *Sec : V)
  InputData.push_back(readDwarf(Sec));
1794–1795

When you bitwise-or some values, you usually want to write values from MSB to LSB.

1802

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.)

1825–1829

This does not "find" anything. It computes. get, compute or calculate are good choices.

1832

Addr -> Address

ELF/SyntheticSections.h
519–524

IndexData is not a good variable name.

grimar updated this revision to Diff 100408.May 26 2017, 7:48 AM
  • Addressed review comments, added more comments.
ELF/GdbIndex.h
54–55

Yes, fixed.

63

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" ?

ELF/SyntheticSections.cpp
1758–1800

Done.

1772–1774

Done.

1794–1795

Fixed in commit of D33551 patch.

1802

Done.

1825–1829

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.

1832

Done.

ELF/SyntheticSections.h
519–524

Renamed.

ruiu added inline comments.May 26 2017, 8:44 AM
ELF/GdbIndex.h
70

Let's call it GdbIndexChunk.

ELF/SyntheticSections.h
512–513

an -> a

512–514

Do you mean that the hash table is a map from names to their types? I don't quite get what "it is a separate area ..." means.

519–524

Thanks. I think this is a good name. :)

grimar added inline comments.May 26 2017, 8:56 AM
ELF/SyntheticSections.h
512–514

I was just mean that it is also an area: "A mapped index consists of several areas, laid out in order: ... 5. The symbol table. This is an open-addressed hash table."
(https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html)

Because "is an hash table" itself sounds like some container for something else for me.

grimar updated this revision to Diff 100422.May 26 2017, 9:10 AM
  • Addressed review comments.
ELF/GdbIndex.h
70

Done.

ruiu added inline comments.Jun 5 2017, 3:32 PM
ELF/SyntheticSections.cpp
1793

If CuId is actually a 32-bit type, define it as uint32_t from the beginning.

1810–1811

Remove a blank line.

1875–1876

Be consistent -- don't mix CU and Cu. Use Cu consistently in this patch.

grimar updated this revision to Diff 101523.Jun 6 2017, 1:27 AM
  • Addressed review comments.
ELF/SyntheticSections.cpp
1793

Done.

1810–1811

Done.

1875–1876

Done.

grimar updated this revision to Diff 101741.Jun 7 2017, 8:20 AM
  • Rebased after recent commits done today.
ruiu accepted this revision.Jun 7 2017, 9:48 AM

LGTM

This revision is now accepted and ready to land.Jun 7 2017, 9:48 AM
This revision was automatically updated to reflect the committed changes.