[ELF] Avoid false-positive assert in getErrPlace()

[arichardson]

Summary

This assertion was added as part of D70659 but did not account for .bss
input sections. I noticed that this assert was incorrectly triggering
while building FreeBSD for MIPS64. Fixed by relaxing the assert to also
account for SHT_NOBITS input sections and adjust the test
mips-jalr-non-function.s to link a file with a .bss section first.

[merge_guards_bot]

Unit tests: pass. 61774 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

FTR, SHT_NOBITS sections have nullptr as .data().data() because:

template <class ELFT>
static ArrayRef<uint8_t> getSectionContents(ObjFile<ELFT> &file,
                                            const typename ELFT::Shdr &hdr) {
  if (hdr.sh_type == SHT_NOBITS)
    return makeArrayRef<uint8_t>(nullptr, hdr.sh_size);
  return check(file.getObj().getSectionContents(&hdr));
}

lld/test/ELF/mips-jalr-non-functions.s

14

# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %S/Inputs/common.s -o %t-common.o
# RUN: ld.lld -shared %t-common.o %t.o -o %t.so 2>&1 | FileCheck %s -check-prefix WARNING-MESSAGE

The RUN lines are the only needed change. Changes below (e.g. .comm common_sym,4,4) are unneeded.

[arichardson]

Remove unncessary changes.

I originally made those because I though I could avoid an additional input file, but it appears that the .bss section is only iterated over later. Depending on this order is fragile anyway so adding another file first is better.

[arichardson]

lld/ELF/Target.cpp

98

I could also add the || (isec->type & SHT_NOBITS) to this if? Is that preferable?

[merge_guards_bot]

Unit tests: pass. 61776 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

lld/ELF/Target.cpp

98

Agree. || (isec->type & SHT_NOBITS) here is preferable.

[grimar]

lld/ELF/Target.cpp

98

+1.

lld/test/ELF/mips-jalr-non-functions.s

10

nit: I do not think we mention function names in tests. They might change.
I think just saying that LLD asserted with this test case because of .bss is enough probably.
No need to specify the exact code place.

[arichardson]

Move to if() above and update commment in test

[merge_guards_bot]

Unit tests: pass. 61807 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

LG if @grimar's comment suggestion is addressed.

[grimar]

LGTM too.

[smeenai]

Should this be cherry-picked to the release branch?

[MaskRay]

In D72567#1823153, @smeenai wrote:

Should this be cherry-picked to the release branch?

Looks like a good candidate. Is @hans responsible for pushing a commit to release/10.x?

[hans]

In D72567#1823166, @MaskRay wrote:

In D72567#1823153, @smeenai wrote:

Should this be cherry-picked to the release branch?

Looks like a good candidate. Is @hans responsible for pushing a commit to release/10.x?

Yes. cherry-picked in afbebff6cd7be7329bda4500dbfacfc94ff8edba

lld/ELF/Target.cpp

TargetInfo *getTarget() {
   }
   llvm_unreachable("unknown target machine");
 }
 
 template <class ELFT> static ErrorPlace getErrPlace(const uint8_t *loc) {
   assert(loc != nullptr);
   for (InputSectionBase *d : inputSections) {
     auto *isec = cast<InputSection>(d);
     if (!isec->getParent())
       continue;
 
     const uint8_t *isecLoc =
         Out::bufferStart
             ? (Out::bufferStart + isec->getParent()->offset + isec->outSecOff)
             : isec->data().data();
     if (isecLoc == nullptr) {
-      assert(isa<SyntheticSection>(isec) && "No data but not synthetic?");
+      assert((isa<SyntheticSection>(isec) || (isec->type & SHT_NOBITS)) &&
+             "No data but not synthetic and not SHT_NOBITS?");
       continue;
     }
     if (isecLoc <= loc && loc < isecLoc + isec->getSize())
       return {isec, isec->template getLocation<ELFT>(loc - isecLoc) + ": "};
   }
   return {};
 }
 

lld/test/ELF/mips-jalr-non-functions.s

 # REQUIRES: mips
 ## Check that we ignore R_MIPS_JALR relocations agains non-function symbols.
 ## Older versions of clang were erroneously generating them for function pointers
 ## loaded from any table (not just the GOT) as well as against TLS function
 ## pointers (when using the local-dynamic model), so we need to ignore these
 ## relocations to avoid generating binaries that crash when executed.
 
 # RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %s -o %t.o
-# RUN: ld.lld -shared %t.o -o %t.so 2>&1 | FileCheck %s -check-prefix WARNING-MESSAGE
+## Link in another object file with a .bss as a regression test:
+## Previously getErrPlace() would assert when skipping over .bss sections.
+## By adding another file with a .bss section before the actual %t.o we can
+## reproduce this case.
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %S/Inputs/common.s -o %t-common.o
+# RUN: ld.lld -shared %t-common.o %t.o -o %t.so 2>&1 | FileCheck %s -check-prefix WARNING-MESSAGE
 # RUN: llvm-objdump --no-show-raw-insn --no-leading-addr -d %t.so | FileCheck %s
 
 .set  noreorder
 test:
   .reloc .Ltmp1, R_MIPS_JALR, tls_obj
 .Ltmp1:
   jr  $t9
   nop