[PATCH] D14382: [ELF2] - Basic implementation of -r/--relocatable

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:1402-1403
@@ -1377,3 +1401,4 @@
                      });
-    return;
+    if (!Config->Relocatable)
+      return;
   }
----------------
ruiu wrote:
> Please try to separate the control flow for -r from the regular control flow. (Or, in general, please do not try to add small `if`s for various flags to achieve something.)
> 
> !StrTabSec.isDynamic() does not make sense for -r. As I understand, what you want to do is to set DynsymIndex for each symbol if -r is given, so that you can use the indices in fixRelocations() later. In that case, you don't actually want to sort anything (is this correct?). So, please add
> 
>   if (Config->Relocatable) {
>     size_t I = 0;
>     for (const std::pair<SymbolBody *, size_t> &P : Symbols)
>       P.first->DynsymIndex = ++I;
>     return;
>   }
> 
> before `if (!StrTabSec.isDynamic())`.
Yes, it looks I dont need sorting. Fixed.

================
Comment at: ELF/OutputSections.cpp:1410
@@ -1384,3 +1409,3 @@
     std::stable_sort(Symbols.begin(), Symbols.end(), sortMipsSymbols);
-  size_t I = 0;
+  size_t I = NumLocals;
   for (const std::pair<SymbolBody *, size_t> &P : Symbols)
----------------
ruiu wrote:
> I do not understand -- isn't this an incompatible change if you do not use -r?
No, it`s not:

When I don't use -r, SymTab execution flow returns earlier, inside if (!StrTabSec.isDynamic()) {...}
For DynSymTab case NumLocals is always 0, so it has no any impact on logic.


================
Comment at: ELF/OutputSections.h:305
@@ +304,3 @@
+  StaticRelocSection(StringRef Name, uint32_t Type, uintX_t Flags, bool IsRela);
+  void addSection(InputSectionBase<ELFT> *C) override;
+  void finalize() override;
----------------
ruiu wrote:
> Do not use C for InputSections because C stands for Chunk. We have removed Chunk, so it no longer makes sense. I'd name S.
Ok. Btw it is still used in 

```
OutputSection::addSection(InputSectionBase<ELFT> *C) override;
```


http://reviews.llvm.org/D14382