[PATCH] D14849: ELF: Make .note.GNU-stack handling compatible with traditional linkers.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:284
@@ -278,1 +283,3 @@
+    Config->ZExecStack = false;
+
   if (Config->OutputFile.empty())
----------------
grimar wrote:
> Currently with this patch logic next situation can happen:
> Somebody specify -z noexecstack and have one input without gnu section. Output will have executable stack what is wrong because overriding anything looks not to work.
> 
> I would think about overriding not default value but result. So if we specify -z noexecstack then always create PT_GNU_STACK segment, if -z execstack then always dont create it.
This is I think what this code does. If -z noexecstack is given, PT_GNU_STACK segment is always created whether input files have .note.GNU-stack or not. if -z execstack is given, PT_GNU_STACK is not created likewise. The point is that this command line option is handled after all input files are read. 

================
Comment at: ELF/InputFiles.cpp:239
@@ -234,2 +238,3 @@
+    Config->ZExecStack = true;
 }
 
----------------
grimar wrote:
> What if I have too inputs, with and without .note.GNU-stack section ?
> Having logic above ZExecStack will not be set to true and PT_GNU_STACK segment will be created,
> But patch description says that all inputs should have .note.GNU-stack for that.
If you have at least one file that does not have .note.GNU-stack section, ZExecStack is set to true, so PT_GNU_STACK is not created. That is in line with the description, no?

================
Comment at: test/ELF/gnustack.s:30
@@ -46,1 +29,2 @@
 _start:
+.section .note.GNU-stack,""
----------------
grimar wrote:
> Test does not check the case when 1 input has gnu stack section and other does not.
Done.


http://reviews.llvm.org/D14849