[PATCH] D137360: [Linker] Remove nocallback attribute while linking

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a subscriber: TimNN.
tejohnson added a comment.

Few comments below. Also, there is another patch pending (D139209 <https://reviews.llvm.org/D139209>) that also performs attribute updates here, and I am thinking these should be combined into the same helper. Can you work with @TimNN on a uniform location for these attribute updates?



================
Comment at: llvm/lib/Linker/IRMover.cpp:530
   void linkNamedMDNodes();
+  ///  Remove nocallback attribute while linking.
+  void removeAttributes(GlobalValue &GV);
----------------
Probably make the comment more generic, since I envision that this function will be expanded to do other attribute updates. Maybe call it "updateAttributes"?


================
Comment at: llvm/lib/Linker/IRMover.cpp:1531
 
+/// Remove nocallback attribute while linking, because nocallback attribute
+/// indicates that the function is only allowed to jump back into caller's
----------------
Since I envision this function will be expanded for other attribute updates, suggest moving this comment down into the body, where it is removing this attribute.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1536
+/// nocallback attribute might call an imported function. When it's caller
+/// module and important function's module are linked together, the function
+/// with nocallback attribute now jumps back into its caller's module.
----------------
Should "important" be "imported"?

Also, I'm not completely following the situation described in these sentences. Can you give me a more detailed example? I might be able to suggest clearer wording once I understand better.


================
Comment at: llvm/test/Linker/drop-attribute.ll:38
+
+; Test that checks that nocallback attribute on a declaration when a definition is not linked in is dropped.
+; CHECK: declare void @test_nocallback_declaration_definition_not_linked_in(){{$}}
----------------
Is dropping the attribute required in this situation?


================
Comment at: llvm/test/Linker/drop-attribute.ll:45
+
+; CHECK: define void @test_nocallback_declaration_definition_linked_in()
----------------
Move this CHECK up above the declaration to be consistent with the location of the other CHECKs. Also, to confirm that the nocallback is dropped, I think you need to add {{$}} at the end?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137360