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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:1217
+// sophisticated error checking in llvm-readobj because the value of diagnostics
+// is low.
+template <typename ELFT>
----------------
grimar wrote:
> in llvm-readobj?
It was not addressed. It doesn't read right to me:
"...but do not implement sophisticated error checking in llvm-readobj because..."

I guess you have meant "checking in LLD" or "checking like in llvm-readobj".


================
Comment at: lld/ELF/InputFiles.cpp:1235
+            Twine::utohexstr(verneedBuf - obj.base()));
+      return {};
+    }
----------------
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;
}
```


================
Comment at: lld/ELF/InputFiles.cpp:1225
+
+  const uint8_t *verneedBuf = obj.base() + sec->sh_offset;
+  const uint8_t *bufEnd =
----------------
This looks a bit cleaner probably:

```
  ArrayRef<uint8_t> data =
      CHECK(obj.template getSectionContentsAsArray<uint8_t>(sec), this);
  const uint8_t *verneedBuf = data.begin();
```

And then you can just inline `bufEnd` as `data.end()` I think. See below.


================
Comment at: lld/ELF/InputFiles.cpp:1228
+      obj.base() +
+      std::min<size_t>(sec->sh_offset + sec->sh_size, obj.getBufSize());
+  for (unsigned i = 0; i != sec->sh_info; ++i) {
----------------
`sh_offset` for ELF64 is `packed<uint>`, which is `std::conditional_t<Is64, uint64_t, uint32_t>`, i.e. it is `uint64_t`,
so I think `size_t` is not a right thing here.


================
Comment at: lld/ELF/Symbols.cpp:717
 void Symbol::resolveShared(const SharedSymbol &other) {
+
   if (isCommon()) {
----------------
unrelated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80059