This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Postpone evaluation of LMA offset.
ClosedPublic

Authored by grimar on Feb 20 2017, 5:52 AM.

Details

Summary

Previously we evaluated the values of LMA incorrectly for next cases:

.text : AT(ADDR(.text) - 0xffffffff80000000) { ... }
.data : AT(ADDR(.data) - 0xffffffff80000000) { .. }
.init.begin : AT(ADDR(.init.begin) - 0xffffffff80000000) { ... }

Reason was that we evaluated offset when VA was not assigned. For case above
we ended up with 3 loads that has similar LMA and it was incorrect.
That is critical for linux kernel.

Patch updates the offset after VA calculation. That fixes the issue I observing.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Feb 20 2017, 5:52 AM
tpimh added a subscriber: tpimh.Feb 20 2017, 7:20 AM
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Feb 21 2017, 9:57 AM
lld/trunk/ELF/LinkerScript.cpp
568

If you change this to

LMAOffset = [=] { return Cmd->LMAExpr(Dot) - Dot; }

I generally don't like to overuse std::pair because its member names (first and second) sometimes hurts code readability.

lld/trunk/ELF/LinkerScript.h
301

This can be just Expr LMAOffset, right?

grimar added inline comments.Feb 22 2017, 1:50 AM
lld/trunk/ELF/LinkerScript.cpp
568

That would work if I do something like:

if (Cmd->LMAExpr) {
  uintX_t D = Dot;
  LMAOffset = [=] { return Cmd->LMAExpr(D) - D; };
}

Because you can't capture member Dot by value.
Does it look fine for you ?

lld/trunk/ELF/LinkerScript.h
301

I would use std::function<uint64_t()> then. Expr assumes Dot argument which is unused.
And would complicate the initialization to:

LMAOffset = {[=](uint64_t) { return Cmd->LMAExpr(D) - D; }};