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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/InputSection.h:95
@@ +94,3 @@
+  // sections.
+  InputSectionBase<ELFT> *RelocatingSection = nullptr;
+
----------------
rafael wrote:
> Why do you need to store this? You can always use sh_info, no?
> That way you don't add a field to every InputSectionBase.
I tried to do that, but for me there are too much additional code required instead of the single field. I had to add something like
```
template <class ELFT>
void StaticRelocSection<ELFT>::addSection(InputSectionBase<ELFT> *Sec) {
  if (!RelocOutSec) {
    const ObjectFile<ELFT> &File = *Sec->getFile();
    ArrayRef<InputSectionBase<ELFT> *> Sections = File.getSections();
    InputSectionBase<ELFT> *RelocatedSection =
        Sections[Sec->getSectionHdr()->sh_info];
    RelocOutSec = RelocatedSection->OutSec;
  }
```
for places where I was need that. If you insist I will update the patch with such change.
But I would leave code as is for now.

================
Comment at: ELF/OutputSections.cpp:751
@@ +750,3 @@
+template <class ELFT>
+StaticRelocSection<ELFT>::StaticRelocSection(StringRef Name, uint32_t Type,
+                                             uintX_t Flags, bool IsRela)
----------------
rafael wrote:
> This is remarkably similar to OutputSeciton. Can you use that? 
Sorry, I am not sure what do you mean here. If you mean I can inhirit from OutputSeciton then I was already asked to not do that. That was initial plan in earlier revision of this patch.
If you meant something else, could you please give more details about that ?

================
Comment at: ELF/OutputSections.cpp:763
@@ +762,3 @@
+  else if (RelocOutSec != Sec->RelocatingSection->OutSec)
+    error("Sections to which the relocations applies doesn`t match");
+
----------------
rafael wrote:
> Can you get here? If so, add a test, if not, use an assert.
I just assumed that users who will try linker probably will use the release version. So if them meet that error situation, there is no way they see the assert, it will not be debug build for them.
error() is universal way to prompt here. I would use it or fatal() here.


http://reviews.llvm.org/D14382