This is an archive of the discontinued LLVM Phabricator instance.

Differential D24692

Mark ELF sections whose name start with .note as note
ClosedPublic

Authored by phosek on Sep 16 2016, 5:44 PM.

Details

Reviewers
davide
Commits
rG1290355f5ad3: Mark ELF sections whose name start with .note as note
rL282010: Mark ELF sections whose name start with .note as note
Summary

Previously, such sections would be marked as SHT_PROGBITS which makes it impossible to use an initialized C variable declaration to emit an (allocated) ELF note. The new behavior is also consistent with the ELF assembly parser.

There is a related GCC bug 77609.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek updated this revision to Diff 71716.Sep 16 2016, 5:44 PM
phosek retitled this revision from to Mark ELF sections whose name start with .note as note.
phosek updated this object.
phosek set the repository for this revision to rL LLVM.
phosek added subscribers: llvm-commits, phosek.
ruiu added a subscriber: ruiu.Sep 16 2016, 5:47 PM
ruiu added inline comments.
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
154

Shouldn't this be Name == ".note" || Name.startswith(".note.")?

phosek added inline comments.Sep 16 2016, 6:05 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
154

That's definitely more strict so it's probably a better solution. The logic in lib/MC/MCParser/ELFAsmParser.cpp uses Name.startswith(".note"), shall I update that as well?

ruiu added inline comments.Sep 16 2016, 6:07 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
154

I don't know the answer, but I *think* that's the right thing to do.

phosek added inline comments.Sep 18 2016, 6:46 PM
lib/CodeGen/TargetLoweringObjectFileImpl.cpp
154

I checked the behavior of GNU assembler and it's equivalent to the current implementation, i.e. even .notefoo would end up with SHT_NOTE type.

davide added a subscriber: davide.Sep 18 2016, 9:23 PM

Side note, I almost always dislike relying on section name specific logic (in the assembler and in the linker), but this is the way things are. LGTM, modulo one comment.

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
154

Please add a comment here to explain what's going on (maybe add the link to the gcc bug, if you feel it's OK).

davide accepted this revision.Sep 18 2016, 9:23 PM
davide added a reviewer: davide.
This revision is now accepted and ready to land.Sep 18 2016, 9:23 PM
phosek updated this revision to Diff 71974.Sep 20 2016, 1:18 PM
phosek edited edge metadata.
phosek removed rL LLVM as the repository for this revision.
phosek marked an inline comment as done.
davide added a comment.Sep 20 2016, 1:21 PM

lgtm

Closed by commit rL282010: Mark ELF sections whose name start with .note as note (authored by phosek). · Explain WhySep 20 2016, 1:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
CodeGen/
2 lines
test/
CodeGen/
X86/
19 lines

Diff 71716

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Show First 20 Lines • Show All 145 Lines • ▼ Show 20 Lines if (Name == ".tbss" ||
Name.startswith(".llvm.linkonce.tb.")) Name.startswith(".llvm.linkonce.tb."))
return SectionKind::getThreadBSS(); return SectionKind::getThreadBSS();
return K; return K;
} }
static unsigned getELFSectionType(StringRef Name, SectionKind K) { static unsigned getELFSectionType(StringRef Name, SectionKind K) {
if (Name.startswith(".note"))
ruiuUnsubmitted
Not Done
ReplyInline Actions

Shouldn't this be Name == ".note" || Name.startswith(".note.")?

ruiu: Shouldn't this be `Name == ".note" || Name.startswith(".note.")`?
phosekAuthorUnsubmitted
Not Done
ReplyInline Actions

That's definitely more strict so it's probably a better solution. The logic in lib/MC/MCParser/ELFAsmParser.cpp uses Name.startswith(".note"), shall I update that as well?

phosek: That's definitely more strict so it's probably a better solution. The logic in…
ruiuUnsubmitted
Not Done
ReplyInline Actions

I don't know the answer, but I *think* that's the right thing to do.

ruiu: I don't know the answer, but I *think* that's the right thing to do.
phosekAuthorUnsubmitted
Not Done
ReplyInline Actions

I checked the behavior of GNU assembler and it's equivalent to the current implementation, i.e. even .notefoo would end up with SHT_NOTE type.

phosek: I checked the behavior of GNU assembler and it's equivalent to the current implementation, i.e.
davideUnsubmitted
Done
ReplyInline Actions

Please add a comment here to explain what's going on (maybe add the link to the gcc bug, if you feel it's OK).

davide: Please add a comment here to explain what's going on (maybe add the link to the gcc bug, if you…
return ELF::SHT_NOTE;
if (Name == ".init_array") if (Name == ".init_array")
return ELF::SHT_INIT_ARRAY; return ELF::SHT_INIT_ARRAY;
if (Name == ".fini_array") if (Name == ".fini_array")
return ELF::SHT_FINI_ARRAY; return ELF::SHT_FINI_ARRAY;
if (Name == ".preinit_array") if (Name == ".preinit_array")
▲ Show 20 Lines • Show All 976 Lines • Show Last 20 Lines

test/CodeGen/X86/note-sections.ll

  • This file was added.
; RUN: llc -mtriple x86_64-pc-linux < %s | FileCheck %s
%struct.note = type { %struct.Elf32_Nhdr, [7 x i8], %struct.payload }
%struct.Elf32_Nhdr = type { i32, i32, i32 }
%struct.payload = type { i16 }
@foonote = internal constant %struct.note { %struct.Elf32_Nhdr { i32 7, i32 2, i32 17 }, [7 x i8] c"foobar\00", %struct.payload { i16 23 } }, section ".note.foo", align 4
; CHECK: .section .note.foo,"a",@note
; CHECK-NEXT: .p2align 2
; CHECK-NEXT: foonote:
; CHECK-NEXT: .long 7
; CHECK-NEXT: .long 2
; CHECK-NEXT: .long 17
; CHECK-NEXT: .asciz "foobar"
; CHECK-NEXT: .zero 1
; CHECK-NEXT: .short 23
; CHECK-NEXT: .zero 2
; CHECK-NEXT: .size foonote, 24