[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>
----------------
in llvm-readobj?


================
Comment at: lld/ELF/InputFiles.cpp:1219
+template <typename ELFT>
+std::vector<uint32_t> SharedFile::parseVerneed(const ELFFile<ELFT> &obj,
+                                               const typename ELFT::Shdr *sec) {
----------------
I have a feeling that this method does too much here.

In llvm-readelf we have the following method:

```
template <class ELFT>
Expected<std::vector<VerNeed>>
ELFDumper<ELFT>::getVersionDependencies(const Elf_Shdr *Sec) const;
```

One of possible solutions could be to refactor it and move the logic to `ELFFile<ELFT>` so that
it could be reused from LLD.

Or, if not, then the code here could be simplified a bit, see my comments below.


================
Comment at: lld/ELF/InputFiles.cpp:1226
+  const uint8_t *verneedBuf = obj.base() + sec->sh_offset;
+  const uint8_t *bufEnd = obj.base() + obj.getBufSize();
+  for (unsigned i = 0; i != sec->sh_info; ++i) {
----------------
I think you can use sec->sh_size?

```
const uint8_t *secEnd = obj.base() + sec->sh_size;
```

It should be enough to check that we are not going past the end of the section.


================
Comment at: lld/ELF/InputFiles.cpp:1231
+    // size.
+    if (verneedBuf + sizeof(typename ELFT::Verneed) > bufEnd ||
+        uintptr_t(verneedBuf) % sizeof(uint32_t) != 0) {
----------------
`>=` I think.


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


================
Comment at: lld/ELF/InputFiles.cpp:1253
+              Twine::utohexstr(uint16_t(aux->vna_name)) + ") at offset 0x" +
+              Twine::utohexstr(vernauxBuf - obj.base()));
+        return {};
----------------
I am not sure such precise diagnostic is needed for LLD. It could be just: "error parsing SHT_GNU_verneed: broken vna_name" or just "error parsing SHT_GNU_verneed" for all errors here. So you'll be able to use a new helper lambda too.

I think it is imprortant that llvm-readelf/llvm-readobj is able to say what is wrong with the object, but probably no need to overcomplicate the code in LLD to report the details. Alternatively, `getVersionDependencies` code can be moved to `ELFFile<ELFT>` and reused, as I've mentioned earlier. With that LLD would report detailed errors, but do not do the parsing on its side.


================
Comment at: lld/ELF/InputFiles.h:367
+  template <typename ELFT>
+  std::vector<uint32_t> parseVerneed(const llvm::object::ELFFile<ELFT> &obj,
+                                     const typename ELFT::Shdr *sec);
----------------
It should probably be protected/private.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80059