[PATCH] D25960: Read section headers upfront

[Rafael Espíndola via llvm-commits]

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?


> ================
> 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?


>
> ================
> 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.