This is an archive of the discontinued LLVM Phabricator instance.

Add support for dynamic libraries on Hexagon
ClosedPublic

Authored by sidneym on Sep 20 2018, 11:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sidneym created this revision.Sep 20 2018, 11:26 AM
ruiu added inline comments.Sep 20 2018, 2:12 PM
ELF/Arch/Hexagon.cpp
44 ↗(On Diff #166332)

Is this an ABI specification that it doesn't support lazy PLT resolution?

test/ELF/Inputs/hexagon-shared.s
1 ↗(On Diff #166332)

remove the blank line.

sidneym added inline comments.Sep 20 2018, 6:57 PM
ELF/Arch/Hexagon.cpp
44 ↗(On Diff #166332)

The ABI does support Lazy binding,

sidneym added inline comments.Sep 20 2018, 7:16 PM
ELF/Arch/Hexagon.cpp
44 ↗(On Diff #166332)

I can add the header, I didn't because it wasn't getting used.

ruiu added inline comments.Sep 21 2018, 8:52 AM
ELF/Arch/Hexagon.cpp
44 ↗(On Diff #166332)

OK, can you expand the comment a bit? It is not clear to me that why you mentioned musl here. I guess musl is a commonly used libc for Hexagon?

226 ↗(On Diff #166332)

Format.

sidneym updated this revision to Diff 167204.Sep 26 2018, 4:28 PM
sidneym edited the summary of this revision. (Show Details)

Adds a PLT header.
Fixes formatting issue.

ruiu added inline comments.Sep 26 2018, 4:34 PM
ELF/Arch/Hexagon.cpp
235 ↗(On Diff #167204)

We fill gaps with a trap instruction. Is 0x00 a formal trap instruction (e.g. INT3 on x86)? If not, please choose a trap instruction for Hexagon ISA and replace 0x00 with that instruction.

Also, why do we have a padding at end of each PLT entry? Can we simply remove them?

sidneym updated this revision to Diff 167376.Sep 27 2018, 12:34 PM

Swap zero pad with nop insn pad.
Reduce the padding but still bring plt0 to a 16byte alignment. There was no obvious reason for the additional padding and it is not part of the ABI.

sidneym added inline comments.Sep 27 2018, 12:36 PM
ELF/Arch/Hexagon.cpp
232 ↗(On Diff #167376)

OK, the blank line isn't needed.

sidneym added inline comments.Sep 27 2018, 12:39 PM
ELF/Arch/Hexagon.cpp
236 ↗(On Diff #167376)

OK, a trap, fine. Only after I submit do I carefully read the comments.

sidneym updated this revision to Diff 167382.Sep 27 2018, 12:47 PM

Remove the blank line
Put a trap into the gap between the PLT header and plt0.
Update testcase

sidneym added inline comments.Sep 27 2018, 12:55 PM
ELF/Arch/Hexagon.cpp
235 ↗(On Diff #167382)

Above should be a 0x0c, 0xdb, 0x00, 0x54 not 0x0c, 0xdb, 0x00, 0x52 I didn't git add that change prior to my format-patch.

ruiu accepted this revision.Sep 27 2018, 1:27 PM

LGTM

ELF/Arch/Hexagon.cpp
250–251 ↗(On Diff #167382)

Indentation of the comment.

This revision is now accepted and ready to land.Sep 27 2018, 1:27 PM
MaskRay added inline comments.
test/ELF/hexagon-shared.s
2 ↗(On Diff #167382)

Though not strictly required, object files are better suffixed with .o and shared objects .so

5 ↗(On Diff #167382)

This test may not be affected but some might (it happened in the past for a few times)

When a t.so is used to link t. DT_SONAME of t.so will end up in the .dynstr section of t. Its length may change the section size sometimes. FileCheck on t may get different results on some systems.

For intermediate %t3 here, it'd be better to specify -soname t3.so to fix the length of its DT_SONAME.

MaskRay added inline comments.Sep 27 2018, 1:53 PM
test/ELF/hexagon-shared.s
5 ↗(On Diff #167382)

DT_SONAME of t.so will end up in the .dynstr section of t

Correction: DT_SONAME (if exists) or filename

https://github.com/llvm-mirror/lld/tree/master/ELF/Driver.cpp#L232

// DSOs usually have DT_SONAME tags in their ELF headers, and the
// sonames are used to identify DSOs. But if they are missing,
// they are identified by filenames. We don't know whether the new
// file has a DT_SONAME or not because we haven't parsed it yet.
// Here, we set the default soname for the file because we might
// need it later.
//
// If a file was specified by -lfoo, the directory part is not
// significant, as a user did not specify it. This behavior is
// compatible with GNU.

%t is a temporary filename including a directory name, it may be of varying lengths.

sidneym added inline comments.Sep 27 2018, 2:20 PM
ELF/Arch/Hexagon.cpp
250–251 ↗(On Diff #167382)

OK, <<2.

test/ELF/hexagon-shared.s
5 ↗(On Diff #167382)

OK, I will commit using file extensions, .o and .so I will also add -soname to the link.

This revision was automatically updated to reflect the committed changes.