[PATCH] D81988: [ELF] Fix a dyn_cast<Defined>(nullptr) crash if a local symbol appears in InputFile::symbols

Wed Jun 17 09:08:55 PDT 2020

MaskRay marked 5 inline comments as done.
MaskRay added a comment.

In D81988#2097630 <https://reviews.llvm.org/D81988#2097630>, @grimar wrote:

> In D81988#2097485 <https://reviews.llvm.org/D81988#2097485>, @jhenderson wrote:
>
> > would we be better off emitting some kind of warning or error about the out-of-order list earlier on?
>
>
> +1 to this question.

This should be a separate change. llvm-readobj -s should emit a warning as well (like GNU readelf).

================
Comment at: lld/test/ELF/invalid/broken-symtab-duplicate-symbol.test:2
+# REQUIRES: x86
+## Test that we check nullptr entries in InputFile::symbols (which is supposed to be a
+## non-local symbol list). `local` is a nullptr entry.
----------------
grimar wrote:
> I do not think you should mention the particular internal API names.
> This comment should be more general probably. Like:
> 
> ```
> In this test we have a local symbol that follows a global one. This is not correct .... Check that ...
> ```
This test is coupled with the implementation. If the implementation changes, we'll have to adjust the test. Exposing a bit of implementation may be fine to remind a reader to adjust the test. I'll amend the comment a bit.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81988/new/

https://reviews.llvm.org/D81988