Page MenuHomePhabricator

[X86][ELF][CET] Adding the .note.gnu.property ELF section in X86
ClosedPublic

Authored by mike.dvoretsky on May 21 2018, 8:43 AM.

Details

Summary

In preparation for the proposed linker ABI changes (https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf, https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-cet.pdf), this patch enables emission of the .note.gnu.property section to ELF object files when building CET-enabled modules.

Diff Detail

Repository
rL LLVM

Event Timeline

mike.dvoretsky created this revision.May 21 2018, 8:43 AM

Uploaded the version without the test for the section emission. Corrected.

Updated to use full 8/4 byte integers to store the flags rather than using 1 byte and padding.

Enabled readobj to parse sections that use 32 bits for the data on 64-bit targets (like gcc currently does) and added explicit alignment to the section generation.

craig.topper added inline comments.May 25 2018, 10:37 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
3504 ↗(On Diff #148223)

This format string is wrong for a uint64_t. If you set a bad bit in the upper 32-bits this justs prints <unknown flags: 0x0>

grimar added a subscriber: grimar.May 28 2018, 1:46 AM

I think this should be splitted into 2 patches: one for codegen and one for llvm-readobj tool.

llvm/tools/llvm-readobj/ELFDumper.cpp
3476 ↗(On Diff #148606)

That looks odd. ELFT::uint can be 4 or 8 and Elf_Word is 4, so It is the same as

if (DataSize == 4 || DataSize == 8)

I would go with that then.

It is also probably better to return early:

if (DataSize != 4 && DataSize != 8) {
 OS << format("<corrupt length: 0x%x>\n", DataSize);
 break;
}
...
3480 ↗(On Diff #148606)

And here then you can use something like following then
support::endian::read32<ELFT::TargetEndianness>(Data.data()) and support::endian::read64<ELFT::TargetEndianness>(Data.data())

3481 ↗(On Diff #148606)

You can move it below the if (CFProtection == 0) since it is only used later.

3490 ↗(On Diff #148606)

I think you can avoid using if (First) here, because it is always true here, no?

3498 ↗(On Diff #148606)

Actually, it seems you can get rid of the First variable and just check the flags:

if (CFProtection & GNU_PROPERTY_X86_FEATURE_1_IBT) {
  CFProtection &= ~GNU_PROPERTY_X86_FEATURE_1_IBT;
  OS << "IBT";
  if (CFProtection)
    OS << ", ";
}

if (CFProtection & GNU_PROPERTY_X86_FEATURE_1_SHSTK) {
  CFProtection &= ~GNU_PROPERTY_X86_FEATURE_1_SHSTK;
  OS << "SHSTK";
  if (CFProtection)
    OS << ", ";
}

if (CFProtection)
  OS << format("<unknown flags: 0x%x>", CFProtection);

Made changes to the readobj part per comments. Will wait on comment from @craig.topper before splitting the patch.

mike.dvoretsky marked 6 inline comments as done.May 28 2018, 4:48 AM

Go ahead and split the patch.

mike.dvoretsky edited the summary of this revision. (Show Details)

Removed the llvm-readobj part to release as a separate patch.

jhenderson added inline comments.
llvm/lib/Target/X86/X86AsmPrinter.cpp
566–569 ↗(On Diff #148885)

Can't you use EmitBytes to achieve this?

grimar added inline comments.May 30 2018, 2:45 AM
llvm/lib/Target/X86/X86AsmPrinter.cpp
557 ↗(On Diff #148885)

I would move this assert to the beginning of the scope
and also use isArch64Bit for consistency:

if (FeatureFlagsAnd) {
  assert((TT.isArch32Bit() || TT.isArch64Bit()) && ...

But actually.. is it really possible to arch be 16 bits in X86AsmPrinter?
It seems this assert is just redundant. And if that would be possible, it should be
an explicit error probably.

Replaced assertion with a more explicit error at the start of the code path. Changed the emission of the "GNU\0" byte sequence to use EmitBytes with the StringRef constructor specifically to include the trailing zero byte.

The ELF.h file is no longer affected by this patch since D47473 was commited.

mike.dvoretsky marked an inline comment as done.May 30 2018, 9:24 AM
grimar added inline comments.May 31 2018, 1:50 AM
llvm/lib/Target/X86/X86AsmPrinter.cpp
545 ↗(On Diff #149134)

This could be uint8_t, though I think this better be just unsigned.

556 ↗(On Diff #149134)

Do you need this cast?

558 ↗(On Diff #149134)

This better be moved closer to the location of using

// Emitting note header.
int WordSize = TT.isArch64Bit() ? 8 : 4;
EmitAlignment(WordSize == 4 ? 2 : 3);
llvm/test/CodeGen/X86/note-cet-property.ll
1 ↗(On Diff #149134)

Can you add a check for the 32-bit target too? (Since you implement both in patch).

mike.dvoretsky marked 4 inline comments as done.

Changes made per comments.

llvm/lib/Target/X86/X86AsmPrinter.cpp
556 ↗(On Diff #149134)

Unfortunately, yes. The compilation fails without it.

grimar added inline comments.Jun 1 2018, 5:23 AM
llvm/lib/Target/X86/X86AsmPrinter.cpp
556 ↗(On Diff #149134)

I think you just need to add
#include "llvm/MC/MCSectionELF.h"
to resolve the failure.

grimar added inline comments.Jun 1 2018, 5:28 AM
llvm/lib/Target/X86/X86AsmPrinter.cpp
547 ↗(On Diff #149431)

This if looks not checked by the test case?

mike.dvoretsky marked 3 inline comments as done.
mike.dvoretsky added a reviewer: grimar.

Added an extra flag to the test, so that both flag-reading paths are covered. Since flags are module-wide, if there need to be tests covering them separately they would have to be put in new files.

grimar accepted this revision.Jun 1 2018, 6:10 AM

LGTM.

This revision is now accepted and ready to land.Jun 1 2018, 6:10 AM
This revision was automatically updated to reflect the committed changes.