[lld] r259444 - Always initialize Out<ELFT> members.

Wed Feb 3 11:13:39 PST 2016

On Wed, Feb 3, 2016 at 9:24 AM, Rui Ueyama <ruiu at google.com> wrote:

> On Tue, Feb 2, 2016 at 10:15 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Mon, Feb 1, 2016 at 4:35 PM, Rui Ueyama via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: ruiu
>>> Date: Mon Feb  1 18:35:49 2016
>>> New Revision: 259444
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259444&view=rev
>>> Log:
>>> Always initialize Out<ELFT> members.
>>>
>>> Instead of leave unused fields as is, set them to nullptr.
>>> Currnetly this is NFC, but if you call writeResults more than
>>> once, you should be able to see the difference.
>>>
>>
>> This might be simpler if Out<ELFT> were a struct. Then you can just do
>> `Out<ELFT> = OutTy<ELFT>();` to reset it in one statement. OutTy would just
>> be a struct with appropriate default member initializers.
>>
>
> It needs the variable template which is a C++14 feature, no?
>

I believe we can do something like this to work around that:

template <typename ELFT>
struct Out {
  static OutTy<ELFT> X;
}

In order to still be able to say `Out<ELFT>::Dynamic`, we need to add some
extra members `static DynamicSection<ELFT> *&Dynamic;` to Out.

I'm mostly concerned because it seems error-prone to duplicate the
initialization of Out<ELFT> both in declaration of Out and also inside of
Writer where we have to remember to reinitialize every field. Personally it
seems like a less bug-prone approach to (until C++14) have the duplication
be in the form of the `static DynamicSection<ELFT> *&Dynamic;` helpers
which are completely mechanical and will cause a compiler error if we
forget one.

In other words, the current status quo is we have information duplicated 3
times for each member of "out":
- `static DynamicSection<ELFT> *Dynamic;` inside class Out
- `template <class ELFT> DynamicSection<ELFT> *Out<ELFT>::Dynamic;`
- (re)initialization code in Writer.cpp that hopefully doesn't forget a
case.

The alternative in the attachment would mean we still have 3 places, but
they are all required for the program to even compile (i.e. cannot be
forgotten/inconsistent):
- `DynamicSection<ELFT> *Dynamic = nullptr;` as a non-static member of
OutTy<ELFT>
- `static DynamicSection<ELFT> *&Dynamic;` inside Out<ELFT> as a convenience
- `template <typename ELFT> DynamicSection<ELFT> *&Out<ELFT>::Dynamic =
Out<ELFT>::X.Dynamic;` to appease the compiler and initialize the static
member of Out

If `Out<ELFT>::X.Dynamic` (2 characters longer than `Out<ELFT>::Dynamic`)
is acceptable for now, we can remove the last two duplications. But
considering the frequent use of `Out<ELFT>`, it may be best to keep it
shorter at the cost of the extra initialization code.

-- Sean Silva

>
>
>> -- Sean Silva
>>
>>
>>>
>>> Modified:
>>>     lld/trunk/ELF/Writer.cpp
>>>
>>> Modified: lld/trunk/ELF/Writer.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Writer.cpp?rev=259444&r1=259443&r2=259444&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/Writer.cpp (original)
>>> +++ lld/trunk/ELF/Writer.cpp Mon Feb  1 18:35:49 2016
>>> @@ -114,6 +114,8 @@ template <class ELFT> void elf2::writeRe
>>>    StringTableSection<ELFT> StrTab(".strtab", false);
>>>    if (!Config->StripAll)
>>>      Out<ELFT>::StrTab = &StrTab;
>>> +  else
>>> +    Out<ELFT>::StrTab = nullptr;
>>>    StringTableSection<ELFT> DynStrTab(".dynstr", true);
>>>    Out<ELFT>::DynStrTab = &DynStrTab;
>>>    GotSection<ELFT> Got;
>>> @@ -121,27 +123,37 @@ template <class ELFT> void elf2::writeRe
>>>    GotPltSection<ELFT> GotPlt;
>>>    if (Target->UseLazyBinding)
>>>      Out<ELFT>::GotPlt = &GotPlt;
>>> +  else
>>> +    Out<ELFT>::GotPlt = nullptr;
>>>    PltSection<ELFT> Plt;
>>>    Out<ELFT>::Plt = &Plt;
>>>    std::unique_ptr<SymbolTableSection<ELFT>> SymTab;
>>>    if (!Config->StripAll) {
>>>      SymTab.reset(new SymbolTableSection<ELFT>(*Symtab,
>>> *Out<ELFT>::StrTab));
>>>      Out<ELFT>::SymTab = SymTab.get();
>>> +  } else {
>>> +    Out<ELFT>::SymTab = nullptr;
>>>    }
>>>    SymbolTableSection<ELFT> DynSymTab(*Symtab, *Out<ELFT>::DynStrTab);
>>>    Out<ELFT>::DynSymTab = &DynSymTab;
>>>    HashTableSection<ELFT> HashTab;
>>>    if (Config->SysvHash)
>>>      Out<ELFT>::HashTab = &HashTab;
>>> +  else
>>> +    Out<ELFT>::HashTab = nullptr;
>>>    GnuHashTableSection<ELFT> GnuHashTab;
>>>    if (Config->GnuHash)
>>>      Out<ELFT>::GnuHashTab = &GnuHashTab;
>>> +  else
>>> +    Out<ELFT>::GnuHashTab = nullptr;
>>>    bool IsRela = shouldUseRela<ELFT>();
>>>    RelocationSection<ELFT> RelaDyn(IsRela ? ".rela.dyn" : ".rel.dyn",
>>> IsRela);
>>>    Out<ELFT>::RelaDyn = &RelaDyn;
>>>    RelocationSection<ELFT> RelaPlt(IsRela ? ".rela.plt" : ".rel.plt",
>>> IsRela);
>>>    if (Target->UseLazyBinding)
>>>      Out<ELFT>::RelaPlt = &RelaPlt;
>>> +  else
>>> +    Out<ELFT>::RelaPlt = nullptr;
>>>    DynamicSection<ELFT> Dynamic(*Symtab);
>>>    Out<ELFT>::Dynamic = &Dynamic;
>>>    EhFrameHeader<ELFT> EhFrameHdr;
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160203/1969fe9a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_staticref.cpp
Type: text/x-c++src
Size: 1433 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160203/1969fe9a/attachment.cpp>