Page MenuHomePhabricator

[ELF][X86_64] Fix corrupted LD -> LE optimization for TLS without PLT

Authored by Lekensteyn on Jan 16 2019, 5:43 AM.



The LD -> LE optimization for Thread-Local Storage without PLT requires
an additional "66" prefix, otherwise the next instruction will be
corrupted, causing runtime misbehavior (crashes) of the linked object.

The other (GD -> IE/LD) optimizations are the same with or without PLT,
but at tests for completeness. Those instructions are copied from

This does not try to address ILP32 (x32) support.


Diff Detail


Event Timeline

Lekensteyn created this revision.Jan 16 2019, 5:43 AM

I think it is good. My comments are below (please wait for Rui's ones though, he might have different opinions I guess).

275 ↗(On Diff #182018)

Should this comment be moved under the if (NextOp == 0xe8) check below it seems?

291 ↗(On Diff #182018)

I would probably do an early return here (to avoid unneccessary read of the Loc[5]):

const uint8_t NextOp = Loc[4];
if (NextOp == 0xe8) {
  memcpy(Loc - 3, Inst, sizeof(Inst));

const uint8_t NextModRm = Loc[5];
302 ↗(On Diff #182018)

I do not think we included the links for the code that do relaxations. At least I did not find it around, so I would remove the last 2 lines for consistency.

305 ↗(On Diff #182018)

LLD code style suggests that you should wrap all branches into `{ ... }' here.

To avoid doing that for single error(... line I would also use early return for this place then:

if (NextOp == 0xff && NextModRm == 0x15) {
    // Convert
    //   leaq  x@tlsld(%rip),%rdi               # 48 8d 3d <Loc>
    //   call *__tls_get_addr@GOTPCREL(%rip)    # ff 15 <disp32>
    // to
    //   .long  0x66666666
    //   movq   %fs:0,%rax
    // See "Table 11.9: LD -> LE Code Transition (LP64)" in
    Loc[-3] = 0x66;
    memcpy(Loc - 2, Inst, sizeof(Inst));

error(getErrorLocation(Loc - 3) +
          "Expected R_X86_64_PLT32 or R_X86_64_GOTPCRELX after R_X86_64_TLSLD");
307 ↗(On Diff #182018)

expected should be lowercase I think.

5 ↗(On Diff #182018)

Do you need --hash-style=sysv?

10 ↗(On Diff #182018)

We often skip checking the exact section index when it is not an intention:

Section {{.*}} .rela.dyn
18 ↗(On Diff #182018)

I do not remember we referred to the spec tables in the tests anywhere earlier, it seems not a too bad idea though,
because TLS optimizations is a pain.

I would either add a link to an x86-64-psABI-1.0.pdf in this test or maybe inlined the tables right here?

Rui, what do you think?

Lekensteyn marked 12 inline comments as done.Jan 16 2019, 7:11 AM
Lekensteyn added inline comments.
275 ↗(On Diff #182018)

Not entirely, only the first two instructions (lea and call) are patched by the code below.

I thought that the third instruction is handled by the DTPOFF blocks below, but I could be mistaken though.

302 ↗(On Diff #182018)

It used to be documented in the past, but then it got moved to ELF/Target.cpp.

Would you like it to be removed here?

305 ↗(On Diff #182018)

Will be fixed in the next patch.

In the past I received different feedback in the past regarding single statement blocks, is there any documention about the style? I can't find guidance on the use of curly braces in blocks in any of:

5 ↗(On Diff #182018)

It does not hurt and seems to be used in other tests as well (the default seems --hash-style=sysv on MIPS and --hash-style=both on others).

If I drop it, then the section number and offsets below will also have to be updated. Any preference whether to keep it or not?

10 ↗(On Diff #182018)

If the section changes, then the offsets will likely have to be updated as well, so it might not save that much. I was inspired by test/ELF/tls-opt-gdie.s. Should I apply your suggestion here?

18 ↗(On Diff #182018)

The references were helpful as I could cross-reference the actual output with the expected output :)
I'll add the URL to this file as well. The left side of the table is listed below while the right side of the table appears in the objdump output here.

(During development of this patch, I did not link with an external symbol which resulted in GD -> IE not being tested correctly, it was treated as GD -> LE.)

grimar added inline comments.Jan 16 2019, 7:46 AM
302 ↗(On Diff #182018)

We should wait for Rui's opinion here I think. He usually has own preferences about comments and documenting the code.

305 ↗(On Diff #182018)

LLD can have its own minor differences/tweaks of style. Usually - if you doubt, just look at the code around,
we are very consistent I believe.

A rule for braces in LLD: if any branch has them, then all of the branches should.
We do not use braces for single lines of code. But see above.

So the following example are correct for LLD:

if (XXX)
if (XXX) {
} else {
5 ↗(On Diff #182018)

We do not include the options that are not needed for the test usually.
Some of our tests contains --hash-style=sysv because when we switched style used by default we did not want to update offsets and things for all tests. So we had to do that. But for a new test, I see no reason to include it.

10 ↗(On Diff #182018)

Up to you.

You can just omit the offsets here, I think it is the best way. See znotext-plt-relocations.s for example.
Using of {{.*}} is generally very common, but I know not of our tests do that.

Lekensteyn marked 8 inline comments as done.

Updated coding style as suggested. Only one issue is still open (whether to remove/change the comment in the source file). Thank you for the feedback!

ruiu added inline comments.Jan 16 2019, 11:06 AM
267–275 ↗(On Diff #182054)

Now this comment is at a wrong place? It looks like this comment explains the entire function, but I believe now this comment explains on the first "if" condition.

Lekensteyn marked an inline comment as done.Jan 16 2019, 11:13 AM
Lekensteyn added inline comments.
267–275 ↗(On Diff #182054)

The comment indeed describes the if (NextOp == 0xe8) { case below, except for the third instruction (related to dtpoff). How is that last instruction handled?

I kept the comment here because I thought that the if (Type == R_X86_DTPOFF*) { blocks took care of it. I can move it down, but this part I did not understand.

ruiu added inline comments.Jan 16 2019, 11:20 AM
267–275 ↗(On Diff #182054)

I mean why don't you move this comment inside if (NextOp == 0xe8) {?

ruiu added inline comments.Jan 16 2019, 11:25 AM
290–291 ↗(On Diff #182054)

When we are rewriting instructions, we don't have to understand the instruction encoding or anything but can just follow the rule. So I'd remove NextOp varaible and use Loc[4] instead. Likewise, remove NextModRm and just use Loc[5].

Lekensteyn marked 6 inline comments as done.

I mean why don't you move this comment inside if (NextOp == 0xe8) {?

Because memcpy only patches the first two instructions:

// Convert
//   leaq bar@tlsld(%rip), %rdi     # 48 8d 3d <Loc>
//   callq __tls_get_addr@PLT       # e8 <disp32>

but not this one (which is why I initially left the comment on top):

//   leaq bar@dtpoff(%rax), %rcx

I've now moved the comment verbatim and inlined the NextOp and NextModRm names.

ruiu accepted this revision.Jan 16 2019, 2:29 PM


281 ↗(On Diff #182150)

Add a blank line.

This revision is now accepted and ready to land.Jan 16 2019, 2:29 PM
Lekensteyn marked an inline comment as done.Jan 16 2019, 2:32 PM

Can I backport this to the 8.0 release branch? Are there any special rules for that?

(Should release notes also be updated?)

281 ↗(On Diff #182150)

Before the if? I will also fix the wrong indentation for the comment.

ruiu added a comment.Jan 16 2019, 2:43 PM

We should cherrypick this patch to 8.0 branch so that 8.0 will be shipped with this fix included. I'll file a bug and do, so no worries.

281 ↗(On Diff #182150)

Yes. I would add a blank line before if so that the variable definition and if doesn't come too close.

Lekensteyn marked 2 inline comments as done.

We should cherrypick this patch to 8.0 branch so that 8.0 will be shipped with this fix included. I'll file a bug and do, so no worries.

Thanks Eli for the information and thank you Rui for taking care of backporting! I'll merge this version now to trunk.

This revision was automatically updated to reflect the committed changes.
ruiu added a comment.Jan 16 2019, 3:50 PM

Cherried pick to 8.0 branch as r351401.