[PATCH] D33485: [ELF] - Do not allow -r to eat comdats.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputFiles.cpp:322
       }
-      break;
+    } break;
     case SHT_SYMTAB:
----------------
grimar wrote:
> ruiu wrote:
> > Format
> It is just formatting in this way :(
I think `break` after `}` is somewhat irregular. Move `break` before `}`.


================
Comment at: ELF/InputFiles.cpp:306-309
+    // We discard comdat sections usually. When -r we should not do that. We
+    // still do deduplication in this case to simplify implementation, because
+    // otherwise merging group sections together would requre additional
+    // regeneration of its contents.
----------------
Move it after `case SHT_GROUP`


================
Comment at: ELF/InputSection.cpp:304
+  // First entry is a flag word, we leave it unchanged.
+  uint32_t Flag = read32(&From[0], Config->Endianness);
+  write32(To++, Flag, Config->Endianness);
----------------
You can remove this temporary variable.


================
Comment at: ELF/OutputSections.cpp:276
 
 static uint64_t getOutFlags(InputSectionBase *S) {
+  if (Config->Relocatable)
----------------
grimar wrote:
> ruiu wrote:
> > This looks a bit tricky. We should turn off SHF_COMPRESSED when we decompress sections.
> May be. This patch just removes SHF_GROUP here.
Please rebase.


================
Comment at: ELF/OutputSections.cpp:122-123
+  // provides signature of the section group.
+  InputSection *First = Sec->Sections[0];
+  elf::ObjectFile<ELFT> *Obj = First->getFile<ELFT>();
+  ArrayRef<SymbolBody *> Symbols = Obj->getSymbols();
----------------
It took for a while to understand this code. What was confusing is that the first section is not special. It is just that when you pass -r, we create an output section for each input section, so there's always only one section in an output file.

Please add

  assert(Config->Relocatable && Sec->Sections.size == 1);

remove `First` variable and use Sec->Sections[0] directly. This should improve readability.


https://reviews.llvm.org/D33485