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

[Rui Ueyama via llvm-commits]

I applied your patch, tweaked it for a while and ended up with
http://reviews.llvm.org/D16864. Can you take a look?

On Wed, Feb 3, 2016 at 11:13 AM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> 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/ecef5f69/attachment.html>