[PATCH] D80059: [ELF] Parse SHT_GNU_verneed and respect versioned undefined symbols in shared objects

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:1235
+            Twine::utohexstr(verneedBuf - obj.base()));
+      return {};
+    }
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > What about introducing a helper lambda:
> > > 
> > > ```
> > > auto checkBuf = [](const uint8_t *Buf, const Twine& Name) {
> > >   if (B < secEnd && uintptr_t(verneedBuf) % sizeof(uint32_t) == 0)
> > >     return true;
> > >   error(... invalid " + Name + " at ... )
> > >   return false;
> > > }
> > > ```
> > > 
> > > It should simplify the code a bit I guess.
> > The similar diagnostics occur twice. For `checkBuf`, we will need:
> > 
> > ```
> > if (!checkBuf(...))
> >   return false;
> > ```
> > 
> > The end result will be longer.
> I also do not think it is important to use `error()` for invalid objects. In many places
> LLD calls `fatal()`. Then you can simplify the code. E.g (I've not checked it):
> 
> 
> ```
>   ArrayRef<uint8_t> data =
>       CHECK(obj.template getSectionContentsAsArray<uint8_t>(sec), this);
>   const uint8_t *verneedBuf = data.begin();
> 
>   auto checkBuf = [&](uint8_t *buf, size_t size, size_t align) {
>     if (buf + size > data.end() || uintptr_t(buf) % align != 0)
>       fatal(toString(this) + ": unable to parse SHT_GNU_verneed");
>     return buf;
>   };
> 
>   for (unsigned i = 0; i != sec->sh_info; ++i) {
>     auto *vn = reinterpret_cast<const typename ELFT::Verneed *>(
>         checkBuf(verneedBuf, sizeof(typename ELFT::Verneed), sizeof(uint32_t)));
>     const uint8_t *vernauxBuf = verneedBuf + vn->vn_aux;
>     for (unsigned j = 0; j != vn->vn_cnt; ++j) {
>       auto *aux = reinterpret_cast<const typename ELFT::Vernaux *>(checkBuf(
>           vernauxBuf, sizeof(typename ELFT::Vernaux), sizeof(uint32_t)));
>       uint16_t version = aux->vna_other & VERSYM_VERSION;
>       if (version >= verneeds.size())
>         verneeds.resize(version + 1);
>       if (aux->vna_name >= this->stringTable.size())
>         fatal(toString(this) + ": unable to parse SHT_GNU_verneed");
>       verneeds[version] = aux->vna_name;
>       vernauxBuf += aux->vna_next;
>     }
>     verneedBuf += vn->vn_next;
>   }
>   return verneeds;
> }
> ```
Changed `error` to `fatal`. This saves 6 lines. A helper `checkBuf` may still be a bit unnecessary I think. I do want to give a bit more contex (Vernaux parsing error instead of Verneed parsing error).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80059/new/

https://reviews.llvm.org/D80059