[lld] [llvm] ELF: Use index 0 for unversioned undefined symbols (PR #168189)

[Fangrui Song via llvm-commits]

MaskRay wrote:

> Trying to summarise [sourceware.org/bugzilla/show_bug.cgi?id=33577](https://sourceware.org/bugzilla/show_bug.cgi?id=33577)
> 
> * `VER_NDX_LOCAL` for a defined symbol should be used for symbols with STB_LOCAL_BINDING, should these be in the dynamic symbol table for any reason.

STB_LOCAL symbols should not be in .dynsym . In GNU ld, the ia64 port (unsupported by LLVM) has local .dynsym defined symbols for unclear reasons.

> * While not clear from the specification, the intent for undefined symbols is to use `VER_NDX_LOCAL` as if it were called `VER_NDX_NONE` meaning no version.

Right that both Solaris and older GNU ld versions use index 0 (VER_NDX_LOCAL, or VER_NDX_NONE as it should have been called) for unversioned undefined symbols.
While the specification isn't clear, the intent is to treat VER_NDX_LOCAL as meaning "no version".

> * GNU ld, while starting at `VER_NDX_LOCAL` switched to `VER_NDX_GLOBAL` and is now switching back to `VER_NDX_GLOBAL`.

Right.

> * Mold currently rejects undefined symbols with `VER_NDX_LOCAL` and would like more time to update.

Right.

In contrast, LLD has always supported both index 0 and 1 for unversioned undefined symbols. My 2020 change that added versioned symbol recognition (https://reviews.llvm.org/D80059) checks both VER_NDX_LOCAL and VER_NDX_GLOBAL, though test coverage was missing.
The lld/test/ELF/dso-undef-extract-lazy.s change in this PR improves that coverage, and I'm inclined to land it separately since it's unrelated to the producer change.

The `lld/test/ELF/dso-undef-extract-lazy.s` change in this PR is to improve the coverage. If it looks good, I'm inclined to land it separately as it's unrelated to the producer change.

> * GNU ld.so is able to handle `VER_NDX_LOCAL` and `VER_NDX_GLOBAL`.

Yes, likely since 1990+.

> Pragmatically [1] I think it would be best to bring ourselves in line with the GNU tools, which would mean merging this. Before I approve can I check a few things:
> 
> * Are we confident that other dynamic linkers that handle symbol versions (bionic, BSD rtld) can handle this change? At a brief glance at the source they look OK, but I haven't thought about it too hard.

glibc has definitely supported index 0 undefined symbols since the 1990s. I believe FreeBSD rtld and Bionic can handle them as well (index into a versym array, and index 0 and index 1 should behave similarly), though I haven't tested this directly.

> * Are we confident that the GNU ld change will stick given Rui's comment. [sourceware.org/bugzilla/show_bug.cgi?id=33577#c23](https://sourceware.org/bugzilla/show_bug.cgi?id=33577#c23) ? I guess we could revert if necessary.
> 
> [1] My reading of the spec was that `VER_NDX_GLOBAL` made more sense given the description of `VERN_NDX_LOCAL` as private.

There's some confusion in that thread. I want to emphasize that using VER_NDX_LOCAL is the traditional GNU ld behavior we're returning to.

I'll wait to land the LLD change until the discussion settles.


https://github.com/llvm/llvm-project/pull/168189