This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf] Symbol index in symbol table printing is not reset
ClosedPublic

Authored by mikkqu on Jul 26 2020, 1:53 PM.

Details

Diff Detail

Event Timeline

mikkqu created this revision.Jul 26 2020, 1:53 PM
lattner resigned from this revision.Jul 26 2020, 2:13 PM

Wow, yeah, static variables are wildly inappopriate for that. That said, I'm not familiar with the code, so someone else should review.

grimar added a subscriber: grimar.Jul 27 2020, 1:40 AM

Generally looks good. A few comments inlined.
But please do not upload patches without a context, see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

llvm/test/tools/llvm-readobj/ELF/section-symbols-index.test
1 ↗(On Diff #280750)

Probably this test should be a part of symbols.test.

4 ↗(On Diff #280750)

We prefer long form for options in tests I think. I.e. -s -> --symbols.

18 ↗(On Diff #280750)

I'd expect to have just 6 lines here probably. E.g:

# CHECK:      Symbol table '.symtab' contains 3 entries:
# CHECK-NEXT:  Num: {{.*}}
# CHECK-NEXT:    0: {{.*}}
# CHECK:      Symbol table '.symtab' contains 3 entries:
# CHECK-NEXT:  Num: {{.*}}
# CHECK-NEXT:    0: {{.*}}
30 ↗(On Diff #280750)

You can just have Symbols: [] here. It is enough for the test.

llvm/tools/llvm-readobj/ELFDumper.cpp
3932

You can inline it:

Fields[0].Str = to_string(format_decimal(Symbol - FirstSym, 6)) + ":";
mikkqu updated this revision to Diff 280834.EditedJul 27 2020, 3:10 AM

Changes on comments from @grimar:

  • Updated the test case and moved it to symbols.test
  • Inlined the index evaluation
  • Uploaded full context
grimar added inline comments.Jul 27 2020, 6:37 AM
llvm/test/tools/llvm-readobj/ELF/symbols.test
123

You probably do not need a new YAML anymore. You can just reuse the %t64, used in cases 1-5:
llvm-readelf --symbols %t64 %t64

mikkqu updated this revision to Diff 280898.Jul 27 2020, 6:57 AM
  • Reuse YAML in test case
grimar accepted this revision.Jul 28 2020, 12:06 AM

LGTM

This revision is now accepted and ready to land.Jul 28 2020, 12:06 AM
jhenderson accepted this revision.Jul 28 2020, 2:15 AM

Thanks, this change looks good to me too. Do you need @grimar or I to commit it for you?

P.S, you can mark inline comments as "Done" by selecting the appropriate option for them. If you do this before uploading a new diff, it will submit that "Mark as Done" automatically, and it will be easier to see what changes you intended to address with your patch.

Yes, could you please commit it, as I don't have the access.

I can commit it, but I need to know a name and email you'd like to use as author. E.g:

--author "Name Surname <test@gmail.com>"

That is:

--author "Mikhail Kalashnikov <mikkqu@gmail.com>"
This revision was automatically updated to reflect the committed changes.