[PATCH] D25960: Read section headers upfront

[Rui Ueyama via llvm-commits]

On Tue, Oct 25, 2016 at 3:24 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 25 October 2016 at 18:20, Rui Ueyama <ruiu at google.com> wrote:
> > ruiu added a comment.
> >
> > Thank you for doing this. I agree that it should make things easier to
> create linker-synthesized input sections.
> >
> >
> >
> > ================
> > Comment at: ELF/InputSection.cpp:36
> >    if (!File || Hdr->sh_type == SHT_NOBITS)
> > -    return {};
> > +    return makeArrayRef<uint8_t>(nullptr, Hdr->sh_size);
> >    return check(File->getObj().getSectionContents(Hdr));
> > ----------------
> > What is this change for?
>
>
> The size is now always stored in the Data ArrayRef.
>
> > ================
> > Comment at: ELF/InputSection.cpp:842
> >    }
> > +  ArrayRef<uint8_t> Data = makeArrayRef<uint8_t>(nullptr, Size);
> > +  InputSection Ret(SHF_ALLOC | SHF_WRITE, SHT_NOBITS, Alignment, Data);
> > ----------------
> > Is it a common practice to create an ArrayRef with non-zero size and a
> nullptr? It seems odd to me.
>
> It is not too common. The alternative is to store an explicit size and
> pointer in InputSectionData and create the ArrayRef in a getter. Would
> you prefer that?
>

I think I prefer that because an ArrayRef that doesn't refer any array
feels a weird thing.


>
>
> > ================
> > Comment at: ELF/InputSection.h:88
> >
> > +  uintX_t Flags;
> > +  uintX_t Entsize;
> > ----------------
> > Please add a comment to note that they correspond to Elf_Shdr.
>
> Will do.
>
> >
> > ================
> > Comment at: ELF/InputSection.h:119-123
> > +  uintX_t getFlags() const { return Flags; }
> > +  uint32_t getType() const { return Type; }
> > +  uintX_t getEntsize() const { return Entsize; }
> > +  uint32_t getLink() const { return Link; }
> > +  uint32_t getInfo() const { return Info; }
> > ----------------
> > Do you still need these accessors?
>
> No, but can that be in another patch?


Sure.


>
>
> >
> > ================
> > Comment at: ELF/InputSection.h:280-281
> > +  // Common symbols don't belong to any section. But it is easier for us
> > +  // to handle them as if they belong to some input section. So we
> defined
> > +  // this class. CommonInputSection is a virtual singleton class that
> > +  // "contains" all common symbols.
> > ----------------
> > Please update the comment becasue "this class" no longer exists.
>
> Will do.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161025/dd2a330e/attachment.html>