[PATCH] D15191: [ELF] Support PHDRS command

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Writer.cpp:1176
@@ +1175,3 @@
+    layoutCustomPhdrs(VA, FileOff);
+  else {
+    // The first phdr entry is PT_PHDR which describes the program header
----------------
denis-protivensky wrote:
> grimar wrote:
> > Wouldn`t it be better to write something like:
> > 
> > ```
> >  if (hasCustomPhdrs()) {
> >     layoutCustomPhdrs(VA, FileOff);
> >     return;
> >  }
> > 
> > ```
> > And just copy a piece of left logic from below to layoutCustomPhdrs() or even to separate it to another method ?
> > 
> > Also in currect variant bracket is missed:
> > ```
> >  if (hasCustomPhdrs()) {
> > ```
> > 
> No, it's not better, because there already were cases when the common logic below was extended. This would have caused logical issue in the case you propose.
> 
> Concerning the braces - I don't remember us putting them for one-line operator blocks.
(About braces):

I found multiple usings for one-line blocks:
std::string elf2::searchLibrary()
void LinkerScript::run()
void LinkerScript::addFile(StringRef S)
And no using in the way you do. I think we dont use them only if there is no else branch. And always do if it is exist.

================
Comment at: ELF/Writer.cpp:1217
@@ +1216,3 @@
+                                ? (uint32_t)PT_LOAD
+                                : getAmdgpuPhdr(Sec);
+          setPhdr(PH, PTType, Flags, FileOff, VA, 0, Target->getPageSize());
----------------
denis-protivensky wrote:
> grimar wrote:
> > Change is not relative to this patch (fix of formatting).
> It's related, because you don't see that the whole code moved by one tab left (by default, ignore space mode is on). So this is the result of clang-formatting the whole shifted piece.
Yep, I know its moved, I just supposed correct way would be to not format that part here (to format it in separate patch).

================
Comment at: ELF/Writer.cpp:1469
@@ -1301,3 +1468,3 @@
        Config->OutputSections)
-    for (StringRef Name : OutSec.second)
+    for (StringRef Name : OutSec.second.InputSections) {
       InputToOutputSection[Name] = OutSec.first;
----------------
denis-protivensky wrote:
> grimar wrote:
> > There should be brackets I think:
> > 
> > ```
> > for (const std::pair<StringRef, OutputSectionDescription> &OutSec :
> >        Config->OutputSections) {
> > ...
> > ```
> No, I was told to remove these earlier. We don't use braces for blocks that constitute of one operator.
Hmm that wierd for me then :) Because quick search shows that sometimes we do:

Writer<ELFT>::copyLocalSymbols();
void Writer<ELFT>::createSections();


http://reviews.llvm.org/D15191