[PATCH] D80311: [lld][test] Expand testing for dynamic-list and export-dynamic

[Owen Reynolds via Phabricator via llvm-commits]

gbreynoo marked 6 inline comments as done.
gbreynoo added a comment.

Thanks again MaskRay, to avoid confusion surrounding the term "type" I have renamed the test export-symbols and removed the list of symbols, a number of new uses of weak symbols have also been added to the test. I have also split dynamic-list-cpp into a new test due to the dynamic-list-glob.s name change.

Regarding the testing of shared and preemption I have seen that export-dynamic-symbol.s has testing for this regarding --export-dynamic-symbol, whilst --dynamic-list has testing in dynamic-list-preempt-replace-symbol.s and others. Whilst looking into the behaviour of --dynamic-export I struggled to see the same behaviour. Should it also be affecting shared objects in the same way?

I think testing of shared and preemption could be better expanded in similar tests as I described above. I should better describe the initial reasoning for export-symbols.s as the test is a little vague in what exactly it is covering and the misuse of the term type did not help.  The idea was that this would be a test for a number of different symbols to ensure they were being exported, this didn't seem to fit into the existing tests and was not exhaustive but could be good additional coverage. Although a shared object is created in the test, this was only used to check for a symbol that found it's definition elsewhere.



================
Comment at: lld/test/ELF/export-symbol-types.s:9
+##
+## Exported:
+## - undefined symbols
----------------
MaskRay wrote:
> undefined symbols and shared symbols are exported.
> 
> --export-dynamic, --dynamic-list and --export-dynamic-symbol can export additional symbols: non-local STV_DEFAULT symbols
> 
> The list is not accurate because there is overlap between hidden symbols and weak symbols, for example.
> 
> weak symbols are not unconditionally exported.
> 
> Many symbols generated by the linker are not exported (hidden).
You are right that this list was more confusing than helpful, I have removed it.


================
Comment at: lld/test/ELF/export-symbol-types.s:26
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t-elf.o
+## Use --fatal-warnings to confirm no diagnostics are emitted.
----------------
MaskRay wrote:
> MaskRay wrote:
> > Drop `-elf`. It carries no additional information. All object files discussed here are ELF.
> Delete `llvm-mc -filetype=obj -triple=x86_64 %s -o %t-elf.o`
> 
> the object file is the same
The object file created here is different to the one above, one uses the echo as input, the other this file.


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

https://reviews.llvm.org/D80311