This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Implemented R_386_16 and R_386PC16 relocations
ClosedPublic

Authored by grimar on Dec 1 2016, 8:50 AM.

Details

Summary

A program or object file using R_386_8, R_386_16, R_386_PC16 or R_386_PC8
relocations is not conformant to latest ABI. The R_386_16, and R_386_8
relocations truncate the computed value to 16 - bits and 8 - bits
respectively. R_386_PC16 and R_386_16 are used by some
applications, for example by FreeBSD loaders.

Previously we did not take addend in account for these relocation,
counting it as 0, what is wrong and was a reason of hangs.

This patch needed for example for FreeBSD pmbr (protective mbr).

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 79921.Dec 1 2016, 8:50 AM
grimar retitled this revision from to [ELF] - Implemented R_386_16 and R_386PC16 relocations.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, emaste, evgeny777.
grimar updated this revision to Diff 79922.Dec 1 2016, 8:53 AM
  • Uploaded correct testcase.
grimar added a comment.Dec 1 2016, 9:23 AM

Upd: Second issue I mentioned in description relative to D27200 and I think should be patched after it or different solution be landed.

emaste added inline comments.Dec 1 2016, 10:54 AM
ELF/Target.cpp
457–461 ↗(On Diff #79922)

I think this comment ought to be reworded slightly -- the boot loaders indeed do not conform to the latest i386 psABI. I think that will be true for any .code16 assembly, as BIOS bootloaders must be.

ruiu added inline comments.Dec 1 2016, 11:01 AM
ELF/Target.cpp
457–461 ↗(On Diff #79922)

Yeah. I think R_386_16 is not a nonstandard weird relocation but it's just that it's out of scope of i386 psABI. By i386, it really means 32-bit protected mode of i386, and not 8086 mode. So I wouldn't say it's unfortunate that we need to support that.

463 ↗(On Diff #79922)

Don't you need an overflow check?

emaste added inline comments.Dec 1 2016, 12:43 PM
ELF/Target.cpp
457–461 ↗(On Diff #79922)

Maybe something like R_386_PC16 and R_386_16 are not part of the current i386 psABI. They are used by 16-bit x86 objects, like boot loaders?

grimar updated this revision to Diff 80051.Dec 2 2016, 4:01 AM
  • Addressed review comments.
  • Changed testcase to be single file and show how we truncate the value.
ELF/Target.cpp
463 ↗(On Diff #79922)

If you mean some truncation validation, then no.
intel386-psABI-1.1.pdf says:
"The R_386_16, and R_386_8 relocations
truncate the computed value to 16-bits and 8-bits respectively"
It does not say we should do any checks.

I moved the common checkInt<32>(Loc, Val, Type); check above new code though,
it looks reasonable.

ruiu accepted this revision.Dec 2 2016, 11:09 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 2 2016, 11:09 AM
grimar updated this object.Dec 2 2016, 11:39 PM
grimar edited edge metadata.
This revision was automatically updated to reflect the committed changes.