[PATCH] D41234: [ELF] Fix placement of a sentinel entry in the .ARM.exidx section.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin updated this revision to Diff 127464.
ikudrin added a comment.

Thanks for the review, @rafael!

>> - RUN: ld.lld --no-merge-exidx-entries -T %t.script %t.o -shared -o %t.so
>> - RUN: llvm-objdump -s -triple=armv7a-none-linux-gnueabi %t.so | FileCheck %s +// REQUIRES: arm
> 
> Why the change from // to #?

I see that you have unified the comment style, so, in this update I use the same, minimizing the diff.

>> +// RUN: llvm-objdump -s -triple=armv7a-none-linux-gnueabi %t.so | FileCheck %s --check-prefix=DUMP
>  Please use a prefix on the new FileCheck run instead to reduce the diff
>  (or commit the prefix change first).

Fixed.

>> +      if (!Sections.empty() && isa<ARMExidxSentinelSection>(Sections.back())) {
> 
> All tests pass if I move the !Sections.empty() to an assert.

As `removeUnusedSyntheticSections()` is not perfect, I can cook up a malicious script like this:

  --- a/test/ELF/arm-data-prel.s
  +++ b/test/ELF/arm-data-prel.s
  @@ -1,7 +1,8 @@
   // RUN: llvm-mc %s -triple=armv7-unknown-linux-gnueabi -filetype=obj -o %t.o
   // RUN: echo "SECTIONS { \
  -// RUN:          .text : { *(.text) } \
  -// RUN:          .prel.test : { *(.ARM.exidx) } \
  +// RUN:          .text : { *(.text.0) } \
  +// RUN:          .prel.test : { *(.ARM.exidx.0) } \
  +// RUN:          .trap : { *(.ARM.exidx) *(.dummy) } \
   // RUN:          .prel.test.TEST1 : { *(.ARM.exidx.TEST1) } \
   // RUN:          .TEST1 : { *(.TEST1) } } " > %t.script
   // RUN: ld.lld --script %t.script %t.o -o %t
  @@ -32,7 +33,7 @@ _start:
    mov     pc, lr
    .fnend
   
  - .section .text, "ax",%progbits
  + .section .text.0, "ax",%progbits
   // The generated .ARM.exidx section will refer to the personality
   // routine __aeabi_unwind_cpp_pr0. Provide a dummy implementation
   // to stop an undefined symbol error

Generally, I prefer my code to be safe. Although I must admit, that, with this script, lld still crashes, just a bit later, in `OutputSection::finalize()`.

>> +ARMExidxSentinelSection *ARMExidxSentinelSection::create() {
>>  +  if (Config->Relocatable)
>>  +    return nullptr;
>>  +
>>  +  return make<ARMExidxSentinelSection>();
>>  +}
> 
> This is now trivial, please inline.

Done.

> LGTM with that.

Thanks! I'll commit this if there will be no further comments.


https://reviews.llvm.org/D41234

Files:
  ELF/SyntheticSections.cpp
  ELF/SyntheticSections.h
  ELF/Writer.cpp
  test/ELF/arm-exidx-dedup-and-sentinel.s
  test/ELF/linkerscript/arm-exidx-sentinel-and-assignment.s

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41234.127464.patch
Type: text/x-patch
Size: 9085 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171219/065f694d/attachment.bin>