[PATCH] D13244: [ELF2] - implemented --allow-shlib-undefined/--no-allow-shlib-undefined

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Config.h:28
@@ -27,2 +27,3 @@
   bool NoInhibitExec = false;
+  bool AllowShlibUndefines = false;
 };
----------------
denis-protivensky wrote:
> denis-protivensky wrote:
> > `AllowShlibUndefined`
> As I can read from the description provided, this flag has different meanings depending on whether executable or DSO is being linked. If so, it's initial value is neither `false` nor `true`, because it depends on other flags. Hence you should use `Optional` for such a value.
Honestly I dont see advantage of using it here. In continue of that logic - all flags depends on command line arguments :)
We are going to set AllowShlibUndefined flag once at start and only read it later. Optional seems to be too heavy solution for this.
So I would like to hear more opinions about change to Optional here. 

================
Comment at: ELF/Driver.cpp:102
@@ -99,1 +101,3 @@
 
+  if (auto *arg = Args.getLastArg(OPT_allow_shlib_undefined,
+                                  OPT_no_allow_shlib_undefined)) {
----------------
denis-protivensky wrote:
> Better to have two checks of the form:
> 
> ```auto *Arg = Args.getLastArg(OPT_allow_shlib_undefined);
> if (Arg || Config->Shared)
>   Config->AllowShlibUndefines = true;```
> 
> The same is for the contrary option. And remove the code above in `OPT_shared` check.
> Also, watch local variables to start from the capital letter.
What if both flags --allow_shlib_undefined and --no-allow_shlib_undefined be declared at the same time ? My solution will choose the latest in command line I think. Your one will choose one depending of what condition is latest in code. 
Not sure what is better and if we should report an error in this case.

================
Comment at: ELF/Options.td:44
@@ +43,3 @@
+      HelpText<"Do not allow undefined symbols from dynamic"
+               " library when creating executables">;
+
----------------
denis-protivensky wrote:
> Why executables? It affects DSOs as well.
Will be fixed.

================
Comment at: ELF/Writer.cpp:298
@@ -284,9 +297,3 @@
 
-  std::string Message = "undefined symbol: " + Sym.getName().str();
-  if (SymFile)
-    Message += " in " + SymFile->getName().str();
-  if (Config->NoInhibitExec)
-    warning(Message);
-  else
-    error(Message);
+  undefError(Sym.getName(), SymFile ? SymFile->getName() : "");
 }
----------------
denis-protivensky wrote:
> Don't you need to check that this symbol is from DSO, not from object file, for example?
Not here. 
All checks are performed in single undefError function:
void undefError(const Twine &Symbol, const Twine &SymFile) 

DSOs calls it directly in SharedFile<ELFT>::parse()

================
Comment at: ELF/Writer.h:25
@@ -20,1 +24,3 @@
 
+void undefError(const llvm::Twine &Symbol, const llvm::Twine &SymFile);
+
----------------
denis-protivensky wrote:
> Use `StringRef` to pass string arguments.
Could you please explain why ? It looks like Twine arguments used much more often in llvm code.

================
Comment at: test/elf2/allow-shlib-undefined.s:13
@@ +12,3 @@
+# --allow-shlib-undefined but does not link with --no-allow-shlib-undefined
+# RUN: lld -shared --allow-shlib-undefined -flavor gnu2 %t -o %t.so 
+# RUN: not lld -shared --no-allow-shlib-undefined -flavor gnu2 %t -o %t.so 
----------------
denis-protivensky wrote:
> Didn't check linking of DSO without --[no]-allow-shlib-undefined flags.
Actually I did. The second line:
# RUN: lld -shared -flavor gnu2 %t -o %t.so 


http://reviews.llvm.org/D13244