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

[Rafael Avila de Espindola via llvm-commits]

Igor Kudrin via Phabricator <reviews at reviews.llvm.org> writes:

>
> Index: test/ELF/linkerscript/arm-exidx-sentinel-and-assignment.s
> ===================================================================
> --- test/ELF/linkerscript/arm-exidx-sentinel-and-assignment.s
> +++ test/ELF/linkerscript/arm-exidx-sentinel-and-assignment.s
> @@ -1,13 +1,14 @@
> -# REQUIRES: arm
> -# RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
> -# RUN: echo "SECTIONS {                                        \
> -# RUN:         .ARM.exidx 0x1000 : { *(.ARM.exidx*) foo = .; } \
> -# RUN:         .text      0x2000 : { *(.text*) }               \
> -# RUN:       }" > %t.script
> -## We used to crash if the last output section command for .ARM.exidx
> -## was anything but an input section description.
> -# 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 #?

> +// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
> +// RUN: echo "SECTIONS {                                        \
> +// RUN:         .ARM.exidx 0x1000 : { *(.ARM.exidx*) foo = .; } \
> +// RUN:         .text      0x2000 : { *(.text*) }               \
> +// RUN:       }" > %t.script
> +// We used to crash if the last output section command for .ARM.exidx
> +// was anything but an input section description.
> +// 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 --check-prefix=DUMP

Please use a prefix on the new FileCheck run instead to reduce the diff
(or commit the prefix change first).

> -    if (Config->MergeArmExidx && !Config->Relocatable &&
> -        Config->EMachine == EM_ARM && Sec->Type == SHT_ARM_EXIDX) {
> -      // The EHABI for the Arm Architecture permits consecutive identical
> -      // table entries to be merged. We use a simple implementation that
> -      // removes a .ARM.exidx Input Section if it can be merged into the
> -      // previous one. This does not require any rewriting of InputSection
> -      // contents but misses opportunities for fine grained deduplication where
> -      // only a subset of the InputSection contents can be merged.
> -      int Cur = 1;
> -      int Prev = 0;
> -      int N = Sections.size();
> -      while (Cur < N) {
> -        if (isDuplicateArmExidxSec(Sections[Prev], Sections[Cur]))
> -          Sections[Cur] = nullptr;
> -        else
> -          Prev = Cur;
> -        ++Cur;
> +    if (!Config->Relocatable && Config->EMachine == EM_ARM &&
> +        Sec->Type == SHT_ARM_EXIDX) {
> +
> +      if (!Sections.empty() && isa<ARMExidxSentinelSection>(Sections.back())) {

All tests pass if I move the !Sections.empty() to an assert.

> +ARMExidxSentinelSection *ARMExidxSentinelSection::create() {
> +  if (Config->Relocatable)
> +    return nullptr;
> +
> +  return make<ARMExidxSentinelSection>();
> +}

This is now trivial, please inline.

LGTM with that.

Cheers,
Rafael