[PATCH] D92899: [ThinLTO][test] Add visibility related tests

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Couple minor suggestions copied from review for D92900 <https://reviews.llvm.org/D92900>. Also, I think the macho test will fail right now since it references some symbols that don't exist there?



================
Comment at: llvm/test/ThinLTO/X86/visibility-elf.ll:36
+; CHECK: declare hidden void @protected_def_weak_hidden_def()
+;; Currently the visibility is not propagated for an unimported function.
+; CHECK: declare extern_weak void @not_imported()
----------------
Suggest making this comment a bit more explicit, e.g.:

;; Currently the visibility is not propagated onto an unimported function, because we don't have summaries for declarations.


================
Comment at: llvm/test/ThinLTO/X86/visibility-elf.ll:40
+; CHECK: define available_externally hidden void @hidden_def_weak_ref() !thinlto_src_module !0
+;; This can be hidden.
+; CHECK: define available_externally protected void @protected_def_hidden_ref() !thinlto_src_module !0
----------------
Suggest making this comment something like:

;; This can be hidden, but we cannot communicate the declaration's visibility to other modules because
;; declarations don't have summaries, and the IRLinker overrides it when importing the protected def.


================
Comment at: llvm/test/ThinLTO/X86/visibility-macho.ll:36
+; CHECK2: define hidden i32 @hidden_def_weak_def()
+; CHECK2: define protected void @protected_def_weak_def()
+; CHECK2: define protected void @protected_def_weak_hidden_def()
----------------
The protected_* symbols are not defined in this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92899