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

[David Blaikie via llvm-commits]

On Thu, Dec 8, 2016 at 11:12 AM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> clayborg added a comment.
>
> See inlined comments.
>
>
>
> ================
> 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);
> ----------------
> Might be worth putting this pubnames/pubtypes parser into the
> llvm/lib/DebugInfo/DWARF.


+1


> We don't currently have a parser for this there.


llvm-dwarfdump can dump {gnu_,}pubnames sections, so I think we do already
have a parser there - we should use that one (may need some refactoring to
be nice and general - maybe it's just parsing+dumping all bundled up
together)


> 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.
>
>
> ================
> 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;
> ----------------
> Use StringRef here instead of "const char *"?
>
>
> ================
> Comment at: ELF/SyntheticSections.cpp:1420
> +  uint8_t C;
> +  while ((C = *Str++) != 0) {
> +    C = tolower(C);
> ----------------
> Use the StringRef iteration here?
>
>
> ================
> 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);
> +
> ----------------
> 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.
>

That's not really idiomatic to the LLVM codebase - asserts represents
invariants in the code. If those invariants are violated, the behavior of
the program is undefined, untested, etc.

(it'd be like returning on every array index if the array index is out of
bounds - we don't do that, we just take it as read that we as programmers
must ensure the indexes are valid)

Asserts are debugging tools. If we added code to fail gracefully behind
asserts, we wouldn't actually know if any of that worked (we'd never test
it - because any time we hit an assert it'd be a bug we would fix) - it
could just cause the code to go off in some other random direction instead
of the return path.


> ================
> Comment at: ELF/SyntheticSections.cpp:1441
> +      Builder.readPubNamesAndTypes();
> +  for (std::pair<StringRef, uint8_t> &Pair : NamesAndTypes) {
> +    uint32_t Hash = hash(Pair.first.data());
> ----------------
> grimar wrote:
> > clayborg wrote:
> > > use std::tie here with appropriate var names since you did it below?
> > I am not sure what you mean, sorry. Do you meant next form ?
> >
> >   for (auto& p : someInitializingFunction()) {
> >     std::tie(a, b) = p;
> >     // do stuff;
> >   }
> >
> > If so I am probably missing for what.
> Never mind, I was thinking you can use std::tie in the for loop:
> ```
> for (std::tie(Name, Type) : NamesAndTypes) {
> ```
> but you can't.
>

If you declare the variables outside the loop you could - but that's not
always an improvement. C++17 I think maybe gets the ability to do a
multi-declaration/tie thing.


>
>
> ================
> 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);
> ----------------
> 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.
>
>
> ================
> Comment at: ELF/SyntheticSections.cpp:1522
> +    Buf += 4;
> +    for (std::pair<uint32_t, uint8_t> &P : CuVec) {
> +      uint32_t Index = P.first;
> ----------------
> clayborg wrote:
> > use std::tie?
> Never mind, can't use std::tie in for loop
>
>
> https://reviews.llvm.org/D26283
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161208/a78cd961/attachment-0001.html>