[PATCH] D37837: Keep some relocations with undefined weak symbols

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Thanks for fixing this.
I have no objections to the way you fixed it, some comments inline. Maybe Rui wants to take a look at the style.



================
Comment at: ELF/Config.h:130
   bool HasDynamicList = false;
+  bool HasDynSymTab = false;
   bool ICF;
----------------
you seem to assign this unconditionally, so do you need to initialize this to false?


================
Comment at: ELF/Driver.cpp:1012-1013
 
+  Config->HasDynSymTab = !SharedFile<ELFT>::Instances.empty() || Config->Pic ||
+                         Config->ExportDynamic;
+
----------------
Maybe you can add a comment here? I remember the rule now, but I won't in a bit :)


================
Comment at: ELF/Relocations.cpp:572
   // This relocation would require the dynamic linker to write a value to read
-  // only memory. We can hack around it if we are producing an executable and
+  // only memory. If it is a weak undef, degrade it to zero.
+  if (Body.isUndefWeak()) {
----------------
I see what you mean here, but, do we have a better way of saying "degrade it to zero?"
Something like, force its value to be zero? (Sorry, I don't have a better way of expressing it)


https://reviews.llvm.org/D37837