"raw" patch adding comdat support to lld elf2

[Rui Ueyama via llvm-commits]

You are using SymbolTable's hash table both for dedup'ing SHT_GROUP
sections and store symbols, but seems like the two uses don't overlap in
your code. SHT_GROUP sections only cares if the LSB bit is set in the
pointer, and symbols only care about pointers stored at MSBs. Is this
different from having a separate hash table for SHT_GROUP sections?

On Thu, Oct 8, 2015 at 3:40 PM, Rui Ueyama <ruiu at google.com> wrote:

> Overall looking good. Nice use of PointerIntPair.
>
>    for (const Elf_Shdr &Sec : Obj.sections()) {
> +    ++I;
> +    if (Sections[I] == &InputSection<ELFT>::Discarded)
> +      continue;
> +
>      switch (Sec.sh_type) {
> +    case SHT_GROUP: {
> +      Sections[I] = &InputSection<ELFT>::Discarded;
> +      uint32_t SymtabdSectionIndex = Sec.sh_link;
> +      ErrorOr<const Elf_Shdr *> SecOrErr = Obj.getSection(SymtabdSectionIndex);
> +      error(SecOrErr);
> +      const Elf_Shdr *SymtabSec = *SecOrErr;
> +      uint32_t SymIndex = Sec.sh_info;
> +      const Elf_Sym *Sym = Obj.getSymbol(SymtabSec, SymIndex);
> +      ErrorOr<StringRef> StringTableOrErr =
> +          Obj.getStringTableForSymtab(*SymtabSec);
> +      error(StringTableOrErr);
> +      ErrorOr<StringRef> SignatureOrErr = Sym->getName(*StringTableOrErr);
> +      error(SignatureOrErr);
> +      if (Symtab.insertComdat(*SignatureOrErr))
> +        continue;
> +      typedef support::detail::packed_endian_specific_integral<
> +          uint32_t, ELFT::TargetEndianness, 2> EntryType;
> +      ErrorOr<ArrayRef<EntryType>> EntriesOrErr =
> +          Obj.template getSectionContentsAsArray<EntryType>(&Sec);
> +      error(EntriesOrErr.getError());
> +      ArrayRef<EntryType> Entries = *EntriesOrErr;
> +      if (Entries.empty() || Entries[0] != GRP_COMDAT)
> +        error("Unsupported SHT_GROUP format");
> +      for (EntryType E : Entries.slice(1)) {
> +        uint32_t SecIndex = E;
> +        if (SecIndex >= Size)
> +          error("Invalid section index in group");
> +        Sections[SecIndex] = &InputSection<ELFT>::Discarded;
> +      }
> +      break;
> +    }
>
>
> I'd define
>
>   StringRef ObjectFile<ELFT>::getShtGroupSignature(const Elf_Shdr &)
>
> and
>
>   ArrayRef<EntryType> ObjectFile<ELFT>::getShtGroupEntries(const Elf_Shdr
> &)
>
> so that I can write like this
>
>   Sections[I] = &InputSection<ELFT>::Discarded;
>   if (Symtab.insertComdat(getShtGroupSignature(Sec))
>     continue;
>   for (EntryType E : getShtGroupEntries(Sec)) {
>     uint32_t Idx = E;
>     if (Idx >= Size)
>       error(...);
>     Sections[Idx] = &InputSection<ELFT>::Discarded;
>   }
>
> +  typedef llvm::MapVector<StringRef, llvm::PointerIntPair<Symbol *, 1>> MapType;
> +  typedef MapType::const_iterator raw_iter;
> +  struct iter : public llvm::iterator_adaptor_base<
> +                    iter, raw_iter,
> +                    typename std::iterator_traits<raw_iter>::iterator_category,
> +                    const std::pair<StringRef, Symbol *>> {
> +    iter(raw_iter u) : iter::iterator_adaptor_base(u) {}
> +    const std::pair<StringRef, Symbol *> operator*() const {
> +      const std::pair<StringRef, llvm::PointerIntPair<Symbol *, 1>> &V = *I;
> +      std::pair<StringRef, Symbol *> V2(V.first, V.second.getPointer());
> +      return V2;
>
> Why don't you return V2 directly (without using V2)?
>
> On Thu, Oct 8, 2015 at 3:06 PM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> The attached patch adds comdat handling to the new elf lld.
>>
>> The implementation is direct translation to c++ of the rules in
>>
>> http://www.sco.com/developers/gabi/latest/ch4.sheader.html#section_groups
>>
>> The patch is "raw" is that it still has duplicated code and is missing
>> comments.
>>
>> I have attached the patch, run script and benchmark results linking
>> clang, but the summary is
>>
>> gold:
>> * clang is 77 898 792 bytes.
>> * 1.125165215 seconds time elapsed
>>
>> trunk:
>> * clang is 92 442 088 bytes.
>> * 0.495383463 seconds time elapsed
>>
>> patch:
>> * clang is 87 491 896 bytes.
>> * 0.475477433 seconds time elapsed
>>
>> Cheers,
>> Rafael
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151008/4fc689f1/attachment.html>