This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Fix missing ELF st_other field on versioned symbols
ClosedPublic

Authored by adalava on Mar 15 2019, 2:59 PM.

Event Timeline

adalava created this revision.Mar 15 2019, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2019, 2:59 PM
adalava retitled this revision from Fix missing ELF st_other field on versioned symbols to [ELF] Fix missing ELF st_other field on versioned symbols.Mar 15 2019, 3:07 PM
adalava edited the summary of this revision. (Show Details)
adalava added reviewers: hfinkel, emaste.
adalava added subscribers: luporl, dim, sfertile and 4 others.

Example code:

Results Unpatched: (note missing 0x60 before symbol "openattt@FBSD_1.1")

`$ /usr/local/powerpc64-unknown-freebsd12.0/bin/objdump -t test.o
test.o: file format elf64-powerpc-freebsd

SYMBOL TABLE:
0000000000000000 l df *ABS* 0000000000000000 test.c
0000000000000000 l d .toc 0000000000000000 .toc
0000000000000000 *UND* 0000000000000000 .TOC.
0000000000000000 w F .text 000000000000002c 0x60 __impl_openattt
0000000000000000 g O .bss 0000000000000004 a
0000000000000000 g F .text 000000000000002c 0x60 openattt
0000000000000000 w F .text 000000000000002c openattt@FBSD_1.1`

Patched:

`$ /usr/local/powerpc64-unknown-freebsd12.0/bin/objdump -t test.o

test.o: file format elf64-powerpc-freebsd

SYMBOL TABLE:
0000000000000000 l df *ABS* 0000000000000000 test.c
0000000000000000 l d .toc 0000000000000000 .toc
0000000000000000 *UND* 0000000000000000 .TOC.
0000000000000000 w F .text 000000000000002c 0x60 __impl_openattt
0000000000000000 g O .bss 0000000000000004 a
0000000000000000 g F .text 000000000000002c 0x60 openattt
0000000000000000 w F .text 000000000000002c 0x60 openattt@FBSD_1.1`

adalava edited the summary of this revision. (Show Details)Mar 15 2019, 3:28 PM

Looks good, but do you have a test (e.g. in test/MC/PowerPC)?

adalava updated this revision to Diff 198513.May 7 2019, 12:48 PM
adalava edited the summary of this revision. (Show Details)

add test case

Thank you @MaskRay, a test case was added. Please note this patch currently depends on D56586 and D61647 that need to go in first.

Thanks. The test contains some constructs that are not used, e.g. .symver .addrsig. And I think it is usually better to use llvm-readelf/llvm-readobj. I always find the output of llvm-objdump/objdump weird.. If you don't mind, I can rewrite/minimalize the test and commit for you.

adalava added a comment.EditedMay 8 2019, 9:12 AM

Thanks. The test contains some constructs that are not used, e.g. .symver .addrsig. And I think it is usually better to use llvm-readelf/llvm-readobj. I always find the output of llvm-objdump/objdump weird.. If you don't mind, I can rewrite/minimalize the test and commit for you.

Sure, please go ahead, thank you!
Just a reminder that even using llvm-readobj, test case will depends on D56586 in order to work

This works when .set is placed after openattt:, but it doesn't work if .set __impl_openattt, openattt is placed before openattt:. I think this is still worth landing after we have tools to dump st_other (D61718 or a similar change in llvm-readobj).

MaskRay accepted this revision.May 10 2019, 10:05 AM

I'll create ppc64-localentry-symver.s instead. We can add the test that .set is before .localentry when D56586 is committed.

This revision is now accepted and ready to land.May 10 2019, 10:05 AM
This revision was automatically updated to reflect the committed changes.