[PATCH] D27040: [ELF] - Add support for access to most of synthetic sections from linkerscript.

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: ELF/SyntheticSections.cpp:1589-1590
+template <class ELFT> bool VersionTableSection<ELFT>::empty() const {
+  bool HasVerNeed = In<ELFT>::VerNeed->getNeedNum() != 0;
+  return !In<ELFT>::VerDef && !HasVerNeed;
+}
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > grimar wrote:
> > > > ruiu wrote:
> > > > > grimar wrote:
> > > > > > ruiu wrote:
> > > > > > > grimar wrote:
> > > > > > > > ruiu wrote:
> > > > > > > > > grimar wrote:
> > > > > > > > > > ruiu wrote:
> > > > > > > > > > > I don't understand these conditions. Is this the same as the code below?
> > > > > > > > > > > 
> > > > > > > > > > >   if (!In<ELFT>::Verdef)
> > > > > > > > > > >     return true;
> > > > > > > > > > >   return In<ELFT>::VerNeed->empty();
> > > > > > > > > > It is the same as:
> > > > > > > > > > 
> > > > > > > > > > ```
> > > > > > > > > > template <class ELFT> bool VersionTableSection<ELFT>::empty() const {
> > > > > > > > > >   return !In<ELFT>::VerDef && In<ELFT>::VerNeed->empty();
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > So, you meant VersionTable is empty if
> > > > > > > > > 
> > > > > > > > >  - we have a VerDef section,
> > > > > > > > >  - and VerNeed is empty.
> > > > > > > > > 
> > > > > > > > > But why?
> > > > > > > > Mmm, no, it is empty if
> > > > > > > >   - we **do not** have Verdef
> > > > > > > >   - and VerNeed is empty.
> > > > > > > > 
> > > > > > > > 
> > > > > > > Then
> > > > > > > 
> > > > > > >   if (In<ELFT>::VerDef == nullptr)
> > > > > > >     return true;
> > > > > > >   return In<ELFT>::VerNeed->empty();
> > > > > > > 
> > > > > > > This is the same code as I wrote in the first comment.
> > > > > > No, because your code will return **empty** if we **do not** have Verdef but **have** entries in VerNeed
> > > > > I don't know what you are doing here is right, I need to take a look at the original code. Anyways, can you rewrite this code so that it is easy to understand?
> > > > Sure. Just what would you prefer:
> > > > 
> > > > ```
> > > > template <class ELFT> bool VersionTableSection<ELFT>::empty() const {
> > > >   if (!In<ELFT>::VerDef)
> > > >     return In<ELFT>::VerNeed->empty();
> > > >   return false;
> > > > }
> > > > ```
> > > > 
> > > > or
> > > > 
> > > > ```
> > > > template <class ELFT> bool VersionTableSection<ELFT>::empty() const {
> > > >   return !In<ELFT>::VerDef && In<ELFT>::VerNeed->empty();
> > > > }
> > > > ```
> > > > 
> > > I think I know what confused you.
> > > You wrote:
> > > ```
> > > if (!In<ELFT>::Verdef)
> > >   return true;
> > > ```
> > > 
> > > but this is not the same as
> > > 
> > > ```
> > > if (!In<ELFT>::Verdef->empty())
> > >   return true;
> > > ```
> > > 
> > > 
> > This corresponds to https://github.com/llvm-mirror/lld/blob/5313f87928d212b6cd216cb4c7c22448d335e33e/ELF/Writer.cpp#L991, right?
> Yes, sure. That where I took the condition from initially.
OK, then this is consistent. Please replace it with the one line code.


https://reviews.llvm.org/D27040