[PATCH] D42471: [ARM] Fix lld crash introduced by r321154

[Rafael Avila de Espindola via llvm-commits]

vit9696 via Phabricator <reviews at reviews.llvm.org> writes:

> vit9696 updated this revision to Diff 131295.
> vit9696 added a comment.
>
> Updated with suggested test extension by Peter Smith.
>
>
> https://reviews.llvm.org/D42471
>
> Files:
>   ELF/SyntheticSections.cpp
>   ELF/Writer.cpp
>   test/ELF/arm-exidx-discard.s
>
>
> Index: test/ELF/arm-exidx-discard.s
> ===================================================================
> --- test/ELF/arm-exidx-discard.s
> +++ test/ELF/arm-exidx-discard.s
> @@ -0,0 +1,14 @@
> +// RUN: llvm-mc -filetype=obj -triple arm-gnu-linux-eabi -mcpu cortex-a7 -arm-add-build-attributes %s -o %t.o
> +// RUN: echo "ENTRY(__entrypoint) SECTIONS { . = 0x10000; .text : { *(.text .text.*) } /DISCARD/ : { *(.ARM.exidx*) *(.gnu.linkonce.armexidx.*) } }" > %t.script
> +// RUN: ld.lld -T %t.script %t.o -o %t.elf 2>&1
> +// RUN: llvm-readobj -sections %t.elf | FileCheck %s
> +// REQUIRES: arm
> +
> +.globl  __entrypoint
> +__entrypoint:
> +    bx  lr
> +
> +// Check that .ARM.exidx/.gnu.linkonce.armexidx
> +// are correctly removed if they were added.
> +// CHECK-NOT: .ARM.exidx
> +// CHECK-NOT: .gnu.linkonce.armexidx.
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -1309,7 +1309,7 @@
>      if (!SS)
>        return;
>      OutputSection *OS = SS->getParent();
> -    if (!SS->empty() || !OS)
> +    if (!OS || !SS->empty())
>        continue;
>  
>      std::vector<BaseCommand *>::iterator Empty = OS->SectionCommands.end();
> Index: ELF/SyntheticSections.cpp
> ===================================================================
> --- ELF/SyntheticSections.cpp
> +++ ELF/SyntheticSections.cpp
> @@ -2586,6 +2586,8 @@
>  // The sentinel has to be removed if there are no other .ARM.exidx entries.
>  bool ARMExidxSentinelSection::empty() const {
>    OutputSection *OS = getParent();
> +  if (!OS)
> +    return true;
>    for (auto *B : OS->SectionCommands)
>      if (auto *ISD = dyn_cast<InputSectionDescription>(B))
>        for (auto *S : ISD->Sections)

I somewhat dislike the duplicated null check. Whose responsibility it is
to check if the parent is null?

I think with the change it Writer.cpp it is reasonable to say that
empty() is not defined for sections we are not outputting anyway.

The patch with just the change in ELF/Writer.cpp LGTM.

Are you OK with me committing it like that?

Thanks,
Rafael