Page MenuHomePhabricator

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

Authored by arichardson on Jan 12 2020, 6:25 AM.

Details

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.

Diff Detail

Event Timeline

arichardson created this revision.Jan 12 2020, 6:25 AM
Herald added a project: Restricted Project. · View Herald Transcript

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

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.

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 marked an inline comment as done.Jan 13 2020, 1:48 AM
arichardson added inline comments.
lld/ELF/Target.cpp
98

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

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 added inline comments.Jan 13 2020, 10:01 AM
lld/ELF/Target.cpp
98

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

grimar added inline comments.Jan 13 2020, 11:49 PM
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.

Move to if() above and update commment in test

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 accepted this revision.Jan 14 2020, 10:41 AM

LG if @grimar's comment suggestion is addressed.

This revision is now accepted and ready to land.Jan 14 2020, 10:41 AM
grimar accepted this revision.Jan 15 2020, 12:21 AM

LGTM too.

This revision was automatically updated to reflect the committed changes.
arichardson marked 4 inline comments as done.

Should this be cherry-picked to the release branch?

MaskRay added a subscriber: hans.Jan 15 2020, 5:05 PM

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 added a comment.Jan 17 2020, 1:13 AM

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