[PATCH] D73175: [llvm-readobj][test] - Remove --symbols --dyn-syms part from Object/readobj-shared-object.test.

Fri Jan 24 04:08:04 PST 2020

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/symbols.test:5
 ## Case 1: Test how llvm-readobj and llvm-readelf dumps symbols.
+## a) Check relocatable objects.
 # RUN: yaml2obj %s --docnum=1 -o %t64
----------------
grimar wrote:
> jhenderson wrote:
> > It feels weird to me that we test relocatable and shared objects, but not regular ET_EXEC executables. Why do shared objects need specifically testing?
> I'd say it is wierd that we test relocatable object with dynamic symbols (--dyn-symbols, below) as they normally
> do not have them. Perhaps having a test for both ET_DYN and ET_REL does not hurt to document the behavior?
> I am not sure if we should add a test for ET_EXEC as it looks similar to ET_DYN in the context of this patch to me.
> What do you think?
> 
> I'd say it is wierd that we test relocatable object with dynamic symbols (--dyn-symbols, below) as they normally do not have them.

Fair point. I think one or more of the tools may have only allowed options like this for shared objects at one point. I don't know if that's still the case though. Perhaps a comment is needed to explain why we picked those two formats.

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

https://reviews.llvm.org/D73175