This is an archive of the discontinued LLVM Phabricator instance.

[x86/retpoline] Make the external thunk names exactly match the names that happened to end up in GCC.
ClosedPublic

Authored by chandlerc on Feb 6 2018, 5:59 PM.

Details

Summary

This is really unfortunate, as the names don't have much rhyme or reason
to them. Originally in the discussions it seemed fine to rely on aliases
to map different names to whatever external thunk code developers wished
to use but there are practical problems with that in the kernel it turns
out. And since we're discovering this practical problems late and since
GCC has already shipped a release with one set of names, we are forced,
yet again, to blindly match what is there.

Somewhat rushing this patch out for the Linux kernel folks to test and
so we can get it patched into our releases.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Feb 6 2018, 5:59 PM
rnk accepted this revision.Feb 6 2018, 6:46 PM

Looks good!

llvm/lib/Target/X86/X86ISelLowering.cpp
26969 ↗(On Diff #133125)

nit, think this would be cleaner as two switches?

if (Subtarget.useRetpolineExternalThunk()) {
  switch (Ret) {
  case X86::EAX: return "__x86_indirect_thunk_eax";
  case X86::ECX: return "__x86_indirect_thunk_ecx";
  ...
  }
} else {
  switch (Ret) {
  case X86::EAX: return "__llvm_retpoline_eax";
  case X86::ECX: return "__llvm_retpoline_ecx";
  ...
  }
}
This revision is now accepted and ready to land.Feb 6 2018, 6:46 PM
jyknight accepted this revision.Feb 6 2018, 7:06 PM

Should we just pick a name and go with it?

-eric

Should we just pick a name and go with it?

-eric

When it's an internal COMDAT, it seems good to name it in a way that won't actually collide? The symbol name will still show up places.

chandlerc updated this revision to Diff 133140.Feb 6 2018, 7:42 PM

Code review suggestion.

chandlerc marked an inline comment as done.Feb 6 2018, 7:42 PM
chandlerc added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
26969 ↗(On Diff #133125)

Sure.

jyknight added a comment.EditedFeb 6 2018, 7:51 PM

Oops, just realized this won't necessarily always match: gcc would name the thunk __x86_indirect_thunk_eax for 32bit mode, and __x86_indirect_thunk_rax for 64bit mode.

Maybe it's only a theoretical issue because we only use r11 in 64bit mode though?

chandlerc marked an inline comment as done.Feb 6 2018, 7:56 PM

Oops, just realized this won't necessarily always match: gcc would name the thunk __x86_indirect_thunk_eax for 32bit mode, and __x86_indirect_thunk_rax for 64bit mode.

How does this not match? We only use _eax, and only in 32-mode.

Maybe it's only a theoretical issue because we only use r11 in 64bit mode though?

Not even a theoretical issue. LLVM has no code path that uses a register other than r11 in 64-bit mode as an explicit design decision, and we have asserts to that effect.

Maybe it's only a theoretical issue because we only use r11 in 64bit mode though?

Not even a theoretical issue. LLVM has no code path that uses a register other than r11 in 64-bit mode as an explicit design decision, and we have asserts to that effect.

OK, sounds good.

It was not 100% clear only from looking at this patch, as there's no immediately-visible assert that we wouldn't emit __x86_indirect_thunk_e[acd]x with is64Bit().

Maybe it's only a theoretical issue because we only use r11 in 64bit mode though?

Not even a theoretical issue. LLVM has no code path that uses a register other than r11 in 64-bit mode as an explicit design decision, and we have asserts to that effect.

OK, sounds good.

It was not 100% clear only from looking at this patch, as there's no immediately-visible assert that we wouldn't emit __x86_indirect_thunk_e[acd]x with is64Bit().

Sure, I've added asserts to help document this invariant locally.

This revision was automatically updated to reflect the committed changes.