Page MenuHomePhabricator

[llvm-readelf] Fix dumping of SHN_XINDEX symbols
ClosedPublic

Authored by evgeny777 on Apr 12 2019, 6:49 AM.

Details

Summary

The problem was discussed in D60555

There is no test for PRC, OS and RSV kind of symbols. Does anyone know how to generate them?
Test case uses approach suggested by @arichardson (thanks!)

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 created this revision.Apr 12 2019, 6:49 AM

There is no test for PRC, OS and RSV kind of symbols. Does anyone know how to generate them?

Use yaml2obj, with an explicit section index, but actually you don't need to test them as they are already tested by test/tools/llvm-readobj/elf-symbol-shndx.test.

test/tools/llvm-readobj/many-sections.test
1 ↗(On Diff #194869)

Won't this generate COFF output on Windows hosts? I think you need to specify a target triple, and probably a REQUIRES: x86 or similar.

jhenderson added inline comments.Apr 12 2019, 7:15 AM
test/tools/llvm-readobj/many-sections2.s
1 ↗(On Diff #194872)

You'll need a REQUIRES: x86 line

22 ↗(On Diff #194872)

As well as the CHECK-NOT, it's also probably worth checking that the values in the reserved range are printed as regular values.

MaskRay added inline comments.
test/tools/llvm-readobj/many-sections2.s
20 ↗(On Diff #194872)

Delete

.macro generate_values
.endm

generate_values

You don't have to define the immediately-invoked macro.

evgeny777 updated this revision to Diff 194885.Apr 12 2019, 7:31 AM

Addressed comments from @MaskRay

Okay, basically looks fine. Just a couple of small comments.

test/tools/llvm-readobj/many-sections2.s
1 ↗(On Diff #194885)

A brief comment at the start of test describing what this test is testing might help future readers.

20 ↗(On Diff #194885)

Actually, with the positive CHECKS, I don't think you need the negative ones.

25 ↗(On Diff #194885)

Nit: Too many blank lines at EOF?

evgeny777 updated this revision to Diff 194889.Apr 12 2019, 7:59 AM

Addressed comments

jhenderson accepted this revision.Apr 12 2019, 8:47 AM

LGTM, with nit. Might want to get @MaskRay to give thumbs up on the macro stuff, but it's probably fine.

test/tools/llvm-readobj/many-sections2.s
1 ↗(On Diff #194889)

which -> whose

This revision is now accepted and ready to land.Apr 12 2019, 8:47 AM
MaskRay accepted this revision.Apr 13 2019, 5:10 AM
rupprecht added inline comments.Apr 15 2019, 1:54 AM
test/tools/llvm-readobj/many-sections2.s
8 ↗(On Diff #194889)

My only concern with this patch: how long does this test take to run, specifically, how long does llvm-mc take to generate a file with this many sections? I've run into a similar issue in the past, where I wanted to create a test like this, but generating the test file from something like this took an absurd amount of time.

If it's fast, that'd be great (and if so, this patch LGTM). Maybe I'll dust off my old semi-abandoned patch and try to use this...

My only concern with this patch: how long does this test take to run, specifically, how long does llvm-mc take to generate a file with this many sections?

4 seconds in debug build (Core-i7 2600K)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 4:22 AM