[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:1231
+    // size.
+    if (verneedBuf + sizeof(typename ELFT::Verneed) > bufEnd ||
+        uintptr_t(verneedBuf) % sizeof(uint32_t) != 0) {
----------------
grimar wrote:
> `>=` I think.
`>` makes an unlikely but valid situation (`vn_cnt==0`) work.


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


================
Comment at: lld/ELF/InputFiles.cpp:1253
+              Twine::utohexstr(uint16_t(aux->vna_name)) + ") at offset 0x" +
+              Twine::utohexstr(vernauxBuf - obj.base()));
+        return {};
----------------
grimar wrote:
> 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.
I'll keep the parsing logic in LLD but simplify the diagnostics for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80059