Bug 109929 - tgsi_to_nir.c:2111: undefined reference to `gl_nir_lower_samplers_as_deref'
Summary: tgsi_to_nir.c:2111: undefined reference to `gl_nir_lower_samplers_as_deref'
Status: NEW
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords: bisected, regression
Depends on:
Blocks:
 
Reported: 2019-03-07 22:48 UTC by Vinson Lee
Modified: 2019-09-08 09:46 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
build log of mesa with e582e761b7f49d1c0b100289b62442e6295cefef (430.62 KB, text/plain)
2019-03-12 15:02 UTC, Timur Kristóf
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vinson Lee 2019-03-07 22:48:41 UTC
autotools build error

  CXXLD    pipe_msm.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'
Comment 1 Timur Kristóf 2019-03-08 09:45:16 UTC
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
Comment 2 Timur Kristóf 2019-03-08 10:32:23 UTC
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.
Comment 3 Vinson Lee 2019-03-08 21:14:03 UTC
./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
Comment 4 Timur Kristóf 2019-03-12 15:02:38 UTC
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?
Comment 5 Timur Kristóf 2019-03-14 14:46:24 UTC
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.
Comment 6 Vinson Lee 2019-03-18 06:28:05 UTC
(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>
Comment 7 Vinson Lee 2019-03-18 06:33:05 UTC
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'
Comment 8 Timur Kristóf 2019-03-18 09:53:35 UTC
Can you tell me how to build libxatracker.la? Ie. what autotools command to use? Thanks!
Comment 9 Vinson Lee 2019-03-19 21:31:33 UTC
(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
Comment 10 Vinson Lee 2019-03-28 01:16:06 UTC
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 += \
Comment 11 Jan Vesely 2019-03-31 05:15:32 UTC
(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?
Comment 12 Timur Kristóf 2019-04-01 13:08:49 UTC
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.
Comment 13 Jan Vesely 2019-04-01 16:51:23 UTC
(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.
Comment 14 Michel Dänzer 2019-04-01 16:56:39 UTC
(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.
Comment 15 Timur Kristóf 2019-04-02 19:04:24 UTC
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.
Comment 16 Timur Kristóf 2019-04-23 13:53:00 UTC
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.
Comment 17 Jan Vesely 2019-04-24 22:14:03 UTC
(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.
Comment 18 Eric Engestrom 2019-05-02 12:01:21 UTC
(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.
Comment 19 Jan Vesely 2019-05-02 15:29:15 UTC
(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.
Comment 20 Juan A. Suarez 2019-06-11 16:01:53 UTC
Autotools has been removed in Mesa 19.1.0.

Should we close this?
Comment 21 Timur Kristóf 2019-06-11 17:04:49 UTC
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.
Comment 22 Timur Kristóf 2019-09-08 09:46:34 UTC
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?


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.