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

[Denis Protivensky via llvm-commits]

denis-protivensky added inline comments.

================
Comment at: ELF/Config.h:28
@@ -27,2 +27,3 @@
   bool NoInhibitExec = false;
+  bool AllowShlibUndefines = false;
 };
----------------
grimar wrote:
> 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. 
The problem is that in this logic the flag is neither false nor true by default. It's in the intermediate state. There was a discussion to have indeterminate values for such cases. Optional may be not the best case, but leaving the value of false is also a bit odd.

================
Comment at: ELF/Driver.cpp:102
@@ -99,1 +101,3 @@
 
+  if (auto *arg = Args.getLastArg(OPT_allow_shlib_undefined,
+                                  OPT_no_allow_shlib_undefined)) {
----------------
grimar wrote:
> 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.
The linker applies command line arguments in the order it encounters them. So if multiple flags are provided - you need to pick the last. You may use `filtered` to accomplish that.

================
Comment at: ELF/Writer.h:25
@@ -20,1 +24,3 @@
 
+void undefError(const llvm::Twine &Symbol, const llvm::Twine &SymFile);
+
----------------
grimar wrote:
> 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.
I agree that you *may* pass Twine here because you won't store its value inside. I wouldn't do that, but I see other error functions accept Twines, so ok.

================
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 
----------------
grimar wrote:
> 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 
So the comment above is misleading: it states you check three options, but you do check only two of them.


http://reviews.llvm.org/D13244