Bug 109929

This is my mistake, forgot to add the autotools equivalent when I wrote the meson part:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/225/diffs?commit_id=9a834447d652ea50864bb6c32f4ff99ac10d39bf#6749efedc0ac7001e9a35e66fa0e8dfb6e4ff8d3_537_537

Vinson, can you give me some more details on how to reproduce this, such as what commands you used to configure and build mesa?

I tried the following:

./autogen.sh --enable-autotools --enable-debug --enable-gallium-osmesa --disable-gles1 --enable-gles2 --disable-xvmc --with-platforms=x11,drm,surfaceless,wayland --enable-shared-glapi --enable-gbm --enable-llvm --enable-llvm-shared-libs --with-dri-drivers= --with-gallium-drivers=radeonsi,swrast --with-vulkan-drivers= --enable-nine
make -j8

And it builds without issues.

./autogen.sh --enable-autotools --enable-opencl --with-dri-drivers= --with-gallium-drivers=freedreno

9a834447d652ea50864bb6c32f4ff99ac10d39bf is the first bad commit
commit 9a834447d652ea50864bb6c32f4ff99ac10d39bf
Author: Timur Kristóf <timur.kristof@gmail.com>
Date:   Tue Mar 5 18:59:47 2019 +0100

    tgsi_to_nir: Produce optimized NIR for a given pipe_screen.
    
    With this patch, tgsi_to_nir will output NIR that is tailored to
    the given pipe, by reading its capabilities and adjusting the NIR code
    to those capabilities similarly to how glsl_to_nir works.
    
    It also adds an optimization loop that brings the output NIR in line
    with what glsl_to_nir outputs. This is necessary for the same reason
    why glsl_to_nir has its own optimization loop: currently not every
    driver does these optimizations yet.
    
    For uses which cannot pass a pipe_screen we also keep a variant
    called tgsi_to_nir_noscreen which keeps the old behavior.
    
    Signed-Off-By: Timur Kristóf <timur.kristof@gmail.com>
    Tested-by: Andre Heider <a.heider@gmail.com>
    Tested-by: Rob Clark <robdclark@gmail.com>
    Acked-By: Eric Anholt <eric@anholt.net>

:040000 040000 8a3cea38b8872254f1dcf139a50ec76317167923 650d8da3884a85bac808914967ab9ce8db72bed9 M	src
bisect run success

Created attachment 143635 [details]
build log of mesa with e582e761b7f49d1c0b100289b62442e6295cefef

I tried with the commit before that, e582e761b7f49d1c0b100289b62442e6295cefef and mesa doesn't build with autotools. Attached the log. Am I doing something wrong here?

Okay, I think I got it. Can you please try with this patch:
https://gitlab.freedesktop.org/Venemo/mesa/commit/c1efd5b26d210b017118d29ecf16aaaeeb34a420

It gets rid of the problem for me.

(In reply to Timur Kristóf from comment #5)
> Okay, I think I got it. Can you please try with this patch:
> https://gitlab.freedesktop.org/Venemo/mesa/commit/
> c1efd5b26d210b017118d29ecf16aaaeeb34a420
> 
> It gets rid of the problem for me.

This patch fixed the build error for me too.

Tested-by: Vinson Lee <vlee@freedesktop.org>

There is a similar build error with libxatracker.la.

  CXXLD    libxatracker.la
/usr/bin/ld: ../../../../src/gallium/auxiliary/.libs/libgallium.a(tgsi_to_nir.o): in function `ttn_finalize_nir':
src/gallium/auxiliary/nir/tgsi_to_nir.c:2111: undefined reference to `gl_nir_lower_samplers_as_deref'
/usr/bin/ld: src/gallium/auxiliary/nir/tgsi_to_nir.c:2113: undefined reference to `gl_nir_lower_samplers'

Can you tell me how to build libxatracker.la? Ie. what autotools command to use? Thanks!

(In reply to Timur Kristóf from comment #8)
> Can you tell me how to build libxatracker.la? Ie. what autotools command to
> use? Thanks!

./autogen.sh --enable-autotools --enable-opencl --with-dri-drivers= --with-gallium-drivers=freedreno --enable-xa

The diff fixes the entire build for me.

diff --git a/src/gallium/targets/pipe-loader/Makefile.am b/src/gallium/targets/pipe-loader/Makefile.am
index db515e3097b7..807a100a7d0a 100644
--- a/src/gallium/targets/pipe-loader/Makefile.am
+++ b/src/gallium/targets/pipe-loader/Makefile.am
@@ -53,16 +53,14 @@ endif
 
 PIPE_LIBS += \
        $(top_builddir)/src/gallium/auxiliary/libgallium.la \
-       $(top_builddir)/src/compiler/nir/libnir.la \
-       $(top_builddir)/src/util/libmesautil.la \
+       $(top_builddir)/src/compiler/glsl/libglsl.la \
        $(GALLIUM_COMMON_LIB_DEPS)
 
 AM_LDFLAGS = \
        -module \
        -no-undefined \
        -avoid-version \
-       $(GC_SECTIONS) \
-       $(LD_NO_UNDEFINED)
+       $(GC_SECTIONS)
 
 if HAVE_LD_VERSION_SCRIPT
 AM_LDFLAGS += \
diff --git a/src/gallium/targets/xa/Makefile.am b/src/gallium/targets/xa/Makefile.am
index 7b0e9006e06d..2238a7ea81f6 100644
--- a/src/gallium/targets/xa/Makefile.am
+++ b/src/gallium/targets/xa/Makefile.am
@@ -37,16 +37,14 @@ libxatracker_la_LIBADD = \
        $(top_builddir)/src/gallium/state_trackers/xa/libxatracker.la \
        $(top_builddir)/src/gallium/auxiliary/libgalliumvl_stub.la \
        $(top_builddir)/src/gallium/auxiliary/libgallium.la \
-       $(top_builddir)/src/compiler/nir/libnir.la \
-       $(top_builddir)/src/util/libmesautil.la \
+       $(top_builddir)/src/compiler/glsl/libglsl.la \
        $(LIBDRM_LIBS) \
        $(GALLIUM_COMMON_LIB_DEPS)
 
 libxatracker_la_LDFLAGS = \
        -no-undefined \
        -version-number $(XA_MAJOR):$(XA_MINOR):$(XA_PATCH) \
-       $(GC_SECTIONS) \
-       $(LD_NO_UNDEFINED)
+       $(GC_SECTIONS)
 
 if HAVE_LD_VERSION_SCRIPT
 libxatracker_la_LDFLAGS += \

(In reply to Vinson Lee from comment #10)
> The diff fixes the entire build for me.
> 
> diff --git a/src/gallium/targets/pipe-loader/Makefile.am
> b/src/gallium/targets/pipe-loader/Makefile.am
> index db515e3097b7..807a100a7d0a 100644
> --- a/src/gallium/targets/pipe-loader/Makefile.am
> +++ b/src/gallium/targets/pipe-loader/Makefile.am
> @@ -53,16 +53,14 @@ endif
>  
>  PIPE_LIBS += \
>         $(top_builddir)/src/gallium/auxiliary/libgallium.la \
> -       $(top_builddir)/src/compiler/nir/libnir.la \
> -       $(top_builddir)/src/util/libmesautil.la \
> +       $(top_builddir)/src/compiler/glsl/libglsl.la \
>         $(GALLIUM_COMMON_LIB_DEPS)
>  
>  AM_LDFLAGS = \
>         -module \
>         -no-undefined \
>         -avoid-version \
> -       $(GC_SECTIONS) \
> -       $(LD_NO_UNDEFINED)
> +       $(GC_SECTIONS)
>  
>  if HAVE_LD_VERSION_SCRIPT
>  AM_LDFLAGS += \
> diff --git a/src/gallium/targets/xa/Makefile.am
> b/src/gallium/targets/xa/Makefile.am
> index 7b0e9006e06d..2238a7ea81f6 100644
> --- a/src/gallium/targets/xa/Makefile.am
> +++ b/src/gallium/targets/xa/Makefile.am
> @@ -37,16 +37,14 @@ libxatracker_la_LIBADD = \
>         $(top_builddir)/src/gallium/state_trackers/xa/libxatracker.la \
>         $(top_builddir)/src/gallium/auxiliary/libgalliumvl_stub.la \
>         $(top_builddir)/src/gallium/auxiliary/libgallium.la \
> -       $(top_builddir)/src/compiler/nir/libnir.la \
> -       $(top_builddir)/src/util/libmesautil.la \
> +       $(top_builddir)/src/compiler/glsl/libglsl.la \
>         $(LIBDRM_LIBS) \
>         $(GALLIUM_COMMON_LIB_DEPS)
>  
>  libxatracker_la_LDFLAGS = \
>         -no-undefined \
>         -version-number $(XA_MAJOR):$(XA_MINOR):$(XA_PATCH) \
> -       $(GC_SECTIONS) \
> -       $(LD_NO_UNDEFINED)
> +       $(GC_SECTIONS)
>  
>  if HAVE_LD_VERSION_SCRIPT
>  libxatracker_la_LDFLAGS += \

Does this change add glsl compiler to XA state tracker? that sounds rather unfortunate.
I still see:
pipe_r600.so: undefined symbol: _mesa_extension_table
in both pipe_r600 and pipe_radeonsi when using clover.

Am I reading it correctly that https://gitlab.freedesktop.org/mesa/mesa/merge_requests/225/diffs?commit_id=9a834447d652ea50864bb6c32f4ff99ac10d39bf#6749efedc0ac7001e9a35e66fa0e8dfb6e4ff8d3_537_537
adds glsl dependency to every state tracker?

Jan, the commit you linked adds a dependency from the gallium auxiliary lib to the glsl compiler. This is required by tgsi_to_nir for calling gl_nir_lower_samplers and gl_nir_lower_samplers_as_deref, otherwise tgsi_to_nir cannot produce NIR that works on either radeonsi or iris.

I realize this may be unfortunate, but the good thing is that (as far as I see) mesa is linked with --gc-sections which means that the linker will remove the unnecessary code from all binaries where it is not actually used.

Ideally though, only the drivers (and state trackers) that actually call tgsi_to_nir should need the dependency. If you have a suggestion on how to better organize the code to avoid this problem, please let me know.

(In reply to Timur Kristóf from comment #12)
> Jan, the commit you linked adds a dependency from the gallium auxiliary lib
> to the glsl compiler. This is required by tgsi_to_nir for calling
> gl_nir_lower_samplers and gl_nir_lower_samplers_as_deref, otherwise
> tgsi_to_nir cannot produce NIR that works on either radeonsi or iris.

I won't pretend to understand all the details between glsl/tgsi/nir interactions.
are the gl_nir_lower_* passes pipe driver specific? Can't the mesa tracker convert to NIR and run those passes before sending the code to the pipe driver (based on a pipe flag or something)?
requiring all state trackers to provide symbols needed by libglsl bloats non-gl state trackers (omx/vdpau/clover/xa). I tried to fix clover in a similar way to xa patch in this bug, libglsl is not enough, and OCL ICD refused to load the mesa OCL client driver.

At this time I see few ways out:
a) Either the respective pipe drivers need to pack everything (libglsl including deps)
b) every state tracker needs to include necessary symbols
c) we provide dummy implementation of the necessary symbols for state trackers that don't hit the gl_lower paths.

anyone feel free to correct me, my work on CL was mostly outside mesa, so I might have misunderstood the state_tracker/pipe_driver split.

 
> I realize this may be unfortunate, but the good thing is that (as far as I
> see) mesa is linked with --gc-sections which means that the linker will
> remove the unnecessary code from all binaries where it is not actually used.

gc-sections didn't work as expected. I ran into this problem on pipe_r600 which should not have been affected.
 
> Ideally though, only the drivers (and state trackers) that actually call
> tgsi_to_nir should need the dependency. If you have a suggestion on how to
> better organize the code to avoid this problem, please let me know.

I agree the most immediate (semi-)remedy would be to revert the changes to src/gallium/targets/pipe-loader/Makefile.am and apply it only to the respective pipe drivers that need it.

(In reply to Jan Vesely from comment #13)
> anyone feel free to correct me, my work on CL was mostly outside mesa, so I
> might have misunderstood the state_tracker/pipe_driver split.

You're spot on. Code under src/gallium/ referencing gl(sl) symbols is a layering violation.

As discussed with Jan and Jason on IRC, the proper solution will be to attempt to refactor gl_nir_lower_samplers and gl_nir_lower_samplers_as_deref in a way that separates the GLSL specific code. Then switch TTN over to the new, non-GL-specific functions instead of the current ones.

Currently working on a patch.

Can you guys try this branch?

https://gitlab.freedesktop.org/Venemo/mesa/commits/nir-lower-samplers

It seems to work for me at least on radeonsi, but I haven't been able to test it thoroughly yet.

(In reply to Timur Kristóf from comment #16)
> Can you guys try this branch?
> 
> https://gitlab.freedesktop.org/Venemo/mesa/commits/nir-lower-samplers
> 
> It seems to work for me at least on radeonsi, but I haven't been able to
> test it thoroughly yet.

Hi Timur,

thanks for working on this. Since autotools has been removed I'm not sure how valid this bug is.
My guess is that the current behavior would be similar to #109659 (resulting binaries have undefined symbols).
I'll try to take a look over the weekend to see if that's the case and if your patch removes those undefined symbols.
Ideally, meson would use '-Wl,--no-undefined' by default to catch this kind of errors.

(In reply to Jan Vesely from comment #17)
> Ideally, meson would use '-Wl,--no-undefined' by default to catch this kind
> of errors.

It does; it's controlled by `b_lundef`, which is `true` by default, and mesa's `meson.build` doesn't touch it, so the only way it gets disabled is if you do that yourself.

(In reply to Eric Engestrom from comment #18)
> (In reply to Jan Vesely from comment #17)
> > Ideally, meson would use '-Wl,--no-undefined' by default to catch this kind
> > of errors.
> 
> It does; it's controlled by `b_lundef`, which is `true` by default, and
> mesa's `meson.build` doesn't touch it, so the only way it gets disabled is
> if you do that yourself.

figured as much when I looked into it. It doesn't matter what I set, meson didn't detect this bug for anyone.

Autotools has been removed in Mesa 19.1.0.

Should we close this?

The issue is not really autotools specific. Even though meson is able to build it, ideally TTN shouldn't need to use GL(SL), as Michel said above.

The problem has been fixed in MR 1799:
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1799/

TTN no longer depends on libglsl. I think this bug can be closed now unless anyone has any objections?

Closing per comment #22