On Tue, Mar 5, 2024 at 6:53 PM Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > > On Tue, Mar 05, 2024 at 11:14:45AM +0100, David Marchand wrote: > > On Mon, Mar 4, 2024 at 7:45 PM Stephen Hemminger > > <step...@networkplumber.org> wrote: > > > > > > This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71. > > > > > > Tyler found build issues with MSVC and the thash gfni stubs. > > > The problem would be link errors from missing symbols. > > > > Trying to understand this link error. > > Does it come from the fact that rte_thash_gfni/rte_thash_gfni_bulk > > declarations are hidden under RTE_THASH_GFNI_DEFINED in > > rte_thash_gfni.h? > > > > If so, why not always expose those two symbols unconditionnally and > > link with the stub only when ! RTE_THASH_GFNI_DEFINED. > > So I don't have a lot of background of this lib. > > I think we understand that we can't conditionally expose symbols. That's > what windows was picking up because it seems none of our CI's ever end > up with RTE_THASH_GFNI_DEFINED but my local test system did and failed. > (my experiments showed that Linux would complain too if it was defined)
I can't reproduce a problem when I build (gcc/clang) for a target that has GFNI/AVX512F. binutils ld seems to just ignore unknown symbols in the map. With current main: [dmarchan@dmarchan main]$ nm build/lib/librte_hash.so.24.1 | grep rte_thash_gfni 00000000000088b0 T rte_thash_gfni_supported [dmarchan@dmarchan main]$ nm build-nogfni/lib/librte_hash.so.24.1 | grep rte_thash_gfni 00000000000102c0 T rte_thash_gfni 00000000000102d0 T rte_thash_gfni_bulk 000000000000294e t rte_thash_gfni_bulk.cold 0000000000002918 t rte_thash_gfni.cold 000000000000d3c0 T rte_thash_gfni_supported > > If we always expose the symbols then as you point out we have to > conditionally link with the stub otherwise the inline (non-stub) will be > duplicate and build / link will fail. > > I guess the part I don't understand with your suggestion is how we would > conditionally link with just the stub? We have to link with rte_hash to > get the rest of hash and the stub. I've probably missed something here. No we can't, Stephen suggestion is a full solution. > > Since we never had a release exposing the new symbols introduced by > Stephen in question my suggestion was that we just revert for 24.03 so > we don't end up with an ABI break later if we choose to solve the > problem without exports. > > I don't know what else to do, but I think we need to decide for 24.03. I am fully aware that we must fix this for 24.03. I would like to be sure Stephen fix (see v3) works for you, so have a look because I am not able to reproduce an issue and validate the fix myself. -- David Marchand
