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

[Denis Protivensky via llvm-commits]

denis-protivensky added a subscriber: denis-protivensky.

================
Comment at: ELF/Config.h:28
@@ -27,2 +27,3 @@
   bool NoInhibitExec = false;
+  bool AllowShlibUndefines = false;
 };
----------------
`AllowShlibUndefined`

================
Comment at: ELF/Config.h:28
@@ -27,2 +27,3 @@
   bool NoInhibitExec = false;
+  bool AllowShlibUndefines = false;
 };
----------------
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.

================
Comment at: ELF/Driver.cpp:102
@@ -99,1 +101,3 @@
 
+  if (auto *arg = Args.getLastArg(OPT_allow_shlib_undefined,
+                                  OPT_no_allow_shlib_undefined)) {
----------------
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.

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

================
Comment at: ELF/Options.td:48
@@ +47,2 @@
+                            HelpText<"Allow undefined symbols from dynamic"
+                                     " library when creating executables">;
----------------
Ditto.

================
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() : "");
 }
----------------
Don't you need to check that this symbol is from DSO, not from object file, for example?

================
Comment at: ELF/Writer.h:25
@@ -20,1 +24,3 @@
 
+void undefError(const llvm::Twine &Symbol, const llvm::Twine &SymFile);
+
----------------
Use `StringRef` to pass string arguments.

================
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 
----------------
Didn't check linking of DSO without --[no]-allow-shlib-undefined flags.


http://reviews.llvm.org/D13244