[PATCH] D67002: [llvm-nm] - Add a test case for case when we dump a symbol that belongs to a section with a broken sh_name.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: test/tools/llvm-nm/format-sysv-section.test:42
+# RUN: yaml2obj --docnum=2 %s > %t2.o
+# RUN: llvm-nm %t2.o --format=sysv | FileCheck %s --check-prefix=ERR
+
----------------
grimar wrote:
> jhenderson wrote:
> > Do we get any warning message here? If we do, I think we should check it. If we don't, I feel like we should.
> No, we don't have a warning, because the error is silently consumed:
> https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-nm/llvm-nm.cpp#L1102
> 
> There are 16 places around in total where errors are ignored in the same way.
> 
> I did a quick test to check what GNU nm do. It prints nothing, returns 1 and reports an error:
> 
> ```
> umb at ubuntu:~/tests/66$ nm format-sysv-section.test.tmp2.o --format=sysv
> bfd plugin: format-sysv-section.test.tmp2.o: ELF section name out of range
> nm: format-sysv-section.test.tmp2.o: invalid string offset 65535 >= 35 for section `.shstrtab'
> nm: format-sysv-section.test.tmp2.o: invalid string offset 65535 >= 35 for section `.shstrtab'
> nm: format-sysv-section.test.tmp2.o: bad value
> umb at ubuntu:~/tests/66$ echo $?
> 1
> ```
> 
> Given that difference I do not think we should add warnings/errors in this patch, since it only adds a test case for
> a mistype fixed and used to check we don't fail with LLVM_ENABLE_ABI_BREAKING_CHECKS.
> We should probably decide what the behavior we want to have on errors in llvm-nm first?
Sorry, yes, I wasn't saying that a new error should be part of this change.


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

https://reviews.llvm.org/D67002