This is an archive of the discontinued LLVM Phabricator instance.

LLD: Preserve ABI version during linking ELF
ClosedPublic

Authored by kzhuravl on Feb 11 2019, 12:23 AM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

kzhuravl created this revision.Feb 11 2019, 12:23 AM
ruiu added a comment.Feb 11 2019, 10:48 AM

With this patch, lld simply selects the last object file passed via the command line and sets the ABI version field to the value read from the object file. But is this what you actually want? What if all input files except the last one have ABI version 1, and the last one has ABI version 0?

ruiu added a comment.Feb 11 2019, 2:27 PM

I quickly hacked up this patch: https://gist.github.com/rui314/f3cd508b6cbbde763fc13bc6190d7505 Does this look good?

ELF/Writer.cpp
2421

getAbiVersion is intended to be the single place to compute an ABI version number to write to to an output file. You should always call it under any circumstances.

kzhuravl updated this revision to Diff 186341.Feb 11 2019, 2:42 PM

Make sure ABI versions across all object files are the same.

kzhuravl added a comment.EditedFeb 11 2019, 2:46 PM

I quickly hacked up this patch: https://gist.github.com/rui314/f3cd508b6cbbde763fc13bc6190d7505 Does this look good?

The patch you linked will work. Alternatively, can you take a look at the updated patch I just posted?

Is there any reason to not implement this for all targets? In your link you specifically check for EM_AMDGPU...

Also it seems like OSABI is also suffering from the problem you described in your first comment. Is this intentional?

Thanks

ruiu added a comment.Feb 11 2019, 2:57 PM

The interpretation of ABI version of an ELF object file depends on the target machine type. For example, we can think of a target that allows a mix of version X and version Y objects let a linker to create a shim between cross-version function call. At least most ABIs don't define anything about the ABI version, so it is perhaps most natural to ignore the field at all, unless you are linking for a specific target in which the ABI version has a special meaning. Therefore, I'd think code that works only for AMDGPU target is better.

The interpretation of ABI version of an ELF object file depends on the target machine type. For example, we can think of a target that allows a mix of version X and version Y objects let a linker to create a shim between cross-version function call. At least most ABIs don't define anything about the ABI version, so it is perhaps most natural to ignore the field at all, unless you are linking for a specific target in which the ABI version has a special meaning. Therefore, I'd think code that works only for AMDGPU target is better.

Ok, sounds fair. Do you want me to incorporate your patch into this review?

ruiu added a comment.Feb 11 2019, 3:05 PM

Ok, sounds fair. Do you want me to incorporate your patch into this review?

If you find that patch OK, yes.

nhaehnle removed a subscriber: nhaehnle.Feb 12 2019, 1:09 AM
kzhuravl updated this revision to Diff 186547.Feb 12 2019, 2:51 PM

Address review feedback.

ruiu accepted this revision.Feb 12 2019, 3:07 PM

LGTM

This revision is now accepted and ready to land.Feb 12 2019, 3:07 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 3:59 PM