mesa3d-dev

[Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Luca B. <lu...@lu...>]

This is a different approach to solving this problem that the patch
I previously posted, and unlike that, should not cause any problems.

Right now undefined symbols in DRI drivers will still allow the
build to succeed.

As a result, people modifying drivers they cannot test risk creating
unloadable drivers with no easy way of automatically avoiding it.

For instance, the modifications to nv50 for context transfers caused
such an issue recently.

Unfortunately, just adding -Wl,--no-undefined doesn't work, because
the DRI drivers depend on glapi symbols, but do not depend on
libGL.so.1

Adding -lGL is not the correct solution since DRI drivers are not loaded
just by libGL, but also by X and possibly by other clients.

So, this patch simply tries to build an executable linked to the DRI
driver and to libGL.
If the DRI driver contains any undefined symbols not satisfied by its
dependencies or by libGL, this will fail.

This solution does not alter the built drivers, and does not significantly
slow down the build process.

All classic DRI drivers as well as all the Gallium drivers with configure
options compiled successfully with this change.

Thanks to Xavier Chantry <cha...@gm...> and
Michel Daenzer <mi...@da...> for helping with this.
---
 src/gallium/winsys/drm/Makefile.template |    7 +++++--
 src/mesa/drivers/dri/Makefile.template   |    7 +++++--
 src/mesa/drivers/dri/common/dri_test.c   |   11 +++++++++++
 3 files changed, 21 insertions(+), 4 deletions(-)
 create mode 100644 src/mesa/drivers/dri/common/dri_test.c

diff --git a/src/gallium/winsys/drm/Makefile.template b/src/gallium/winsys/drm/Makefile.template
index f4cc0de..9f984f1 100644
--- a/src/gallium/winsys/drm/Makefile.template
+++ b/src/gallium/winsys/drm/Makefile.template
@@ -64,11 +64,14 @@ SHARED_INCLUDES = \
 default: depend symlinks $(TOP)/$(LIB_DIR)/gallium/$(LIBNAME)
 
 $(LIBNAME): $(OBJECTS) $(MESA_MODULES) $(PIPE_DRIVERS) Makefile \
-		$(TOP)/src/mesa/drivers/dri/Makefile.template
-	$(MKLIB) -o $@ -noprefix -linker '$(CC)' -ldflags '$(LDFLAGS)' \
+		$(TOP)/src/mesa/drivers/dri/Makefile.template $(TOP)/src/mesa/drivers/dri/common/dri_test.o
+	$(MKLIB) -o $@.tmp -noprefix -linker '$(CC)' -ldflags '$(LDFLAGS)' \
 		$(OBJECTS) $(PIPE_DRIVERS) \
                 -Wl,--start-group $(MESA_MODULES) -Wl,--end-group \
                  $(DRI_LIB_DEPS) $(DRIVER_EXTRAS)
+	$(CC) -o $@.test $(TOP)/src/mesa/drivers/dri/common/dri_test.o $@.tmp -L$(TOP)/lib -lGL
+	@rm -f $@.test
+	mv $@.tmp $@
 
 $(TOP)/$(LIB_DIR)/gallium:
 	mkdir -p $@
diff --git a/src/mesa/drivers/dri/Makefile.template b/src/mesa/drivers/dri/Makefile.template
index a0c25d2..2b7634b 100644
--- a/src/mesa/drivers/dri/Makefile.template
+++ b/src/mesa/drivers/dri/Makefile.template
@@ -51,9 +51,12 @@ lib: symlinks subdirs depend
 	@$(MAKE) $(LIBNAME) $(TOP)/$(LIB_DIR)/$(LIBNAME)
 
 $(LIBNAME): $(OBJECTS) $(MESA_MODULES) $(EXTRA_MODULES) Makefile \
-		$(TOP)/src/mesa/drivers/dri/Makefile.template
-	$(MKLIB) -o $@ -noprefix -linker '$(CC)' -ldflags '$(LDFLAGS)' \
+		$(TOP)/src/mesa/drivers/dri/Makefile.template $(TOP)/src/mesa/drivers/dri/common/dri_test.o
+	$(MKLIB) -o $@.tmp -noprefix -linker '$(CC)' -ldflags '$(LDFLAGS)' \
 		$(OBJECTS) $(MESA_MODULES) $(EXTRA_MODULES) $(DRI_LIB_DEPS)
+	$(CC) -o $@.test $(TOP)/src/mesa/drivers/dri/common/dri_test.o $@.tmp -L$(TOP)/lib -lGL
+	@rm -f $@.test
+	mv $@.tmp $@
 
 
 $(TOP)/$(LIB_DIR)/$(LIBNAME): $(LIBNAME)
diff --git a/src/mesa/drivers/dri/common/dri_test.c b/src/mesa/drivers/dri/common/dri_test.c
new file mode 100644
index 0000000..3f3dd35
--- /dev/null
+++ b/src/mesa/drivers/dri/common/dri_test.c
@@ -0,0 +1,11 @@
+/* This is just supposed to make sure we get a reference to
+   the driver entry symbol that the compiler doesn't optimize away */
+
+extern char __driDriverExtensions[];
+
+int main(int argc, char** argv)
+{
+   void* p = __driDriverExtensions;
+   /* avoid possible pointer size mismatch warnings */
+   return *(int*)&p;
+}
-- 
1.6.3.3

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Xavier C. <cha...@gm...>]

On Mon, Mar 15, 2010 at 6:36 PM, Luca Barbieri <lu...@lu...> wrote:
> This is a different approach to solving this problem that the patch
> I previously posted, and unlike that, should not cause any problems.
>
> Right now undefined symbols in DRI drivers will still allow the
> build to succeed.
>
> As a result, people modifying drivers they cannot test risk creating
> unloadable drivers with no easy way of automatically avoiding it.
>
> For instance, the modifications to nv50 for context transfers caused
> such an issue recently.
>
> Unfortunately, just adding -Wl,--no-undefined doesn't work, because
> the DRI drivers depend on glapi symbols, but do not depend on
> libGL.so.1
>
> Adding -lGL is not the correct solution since DRI drivers are not loaded
> just by libGL, but also by X and possibly by other clients.
>
> So, this patch simply tries to build an executable linked to the DRI
> driver and to libGL.
> If the DRI driver contains any undefined symbols not satisfied by its
> dependencies or by libGL, this will fail.
>
> This solution does not alter the built drivers, and does not significantly
> slow down the build process.
>
> All classic DRI drivers as well as all the Gallium drivers with configure
> options compiled successfully with this change.
>
> Thanks to Xavier Chantry <cha...@gm...> and
> Michel Daenzer <mi...@da...> for helping with this.
> ---

Just tested, that also works for me.

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Luca B. <lu...@lu...>]

How about applying this?

It should prevent introducing regressions similar to ones that
happened in the past, with very little downside.

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Dan N. <dbn...@gm...>]

On Fri, Mar 19, 2010 at 12:56 PM, Luca Barbieri <lu...@lu...> wrote:
> How about applying this?
>
> It should prevent introducing regressions similar to ones that
> happened in the past, with very little downside.

Can we just put this program in the demos? Or at least just make it a
separate target (make test-link)? It seems excessive to make this part
of the default build path.

--
Dan

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Luca B. <lu...@lu...>]

> Can we just put this program in the demos? Or at least just make it a
> separate target (make test-link)? It seems excessive to make this part
> of the default build path.

The whole purpose is to run this as part of the standard build, so
that the build fails if any driver is unloadable, (i.e. a modification
to it was botched) and the tree hopefully doesn't get pushed to
master.

You can test it separately by just running glxinfo/glxgears, obviously.

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Dan N. <dbn...@gm...>]

On Fri, Mar 19, 2010 at 2:19 PM, Luca Barbieri <lu...@lu...> wrote:
>> Can we just put this program in the demos? Or at least just make it a
>> separate target (make test-link)? It seems excessive to make this part
>> of the default build path.
>
> The whole purpose is to run this as part of the standard build, so
> that the build fails if any driver is unloadable, (i.e. a modification
> to it was botched) and the tree hopefully doesn't get pushed to
> master.
>
> You can test it separately by just running glxinfo/glxgears, obviously.

For developers that makes a lot of sense, but I've never seen any
other projects impose this type of thing on regular users.

--
Dan

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Luca B. <lu...@lu...>]

> For developers that makes a lot of sense, but I've never seen any
> other projects impose this type of thing on regular users.

Why do you see it as an onerous imposition?
It just tries to compile a program linked with a couple of libraries
(the DRI driver, plus libGL) and makes the build fail if that fails.
It doesn't even execute the built program (and could not always do so
even if it were desired, since you could be cross-compiling).

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Jesse B. <jb...@vi...>]

On Fri, 19 Mar 2010 23:05:27 +0100
Luca Barbieri <lu...@lu...> wrote:

> > For developers that makes a lot of sense, but I've never seen any
> > other projects impose this type of thing on regular users.
> 
> Why do you see it as an onerous imposition?
> It just tries to compile a program linked with a couple of libraries
> (the DRI driver, plus libGL) and makes the build fail if that fails.
> It doesn't even execute the built program (and could not always do so
> even if it were desired, since you could be cross-compiling).

This seems to be killing my build at least:

gmake[6]: Entering directory `/home/jbarnes/working/mesa/src/mesa/drivers/dri/i915'
/bin/sh ../../../../../bin/mklib -o i915_dri.so.tmp -noprefix -linker 'gcc' -ldflags '' \
		../common/utils.o ../common/vblank.o ../common/dri_util.o ../common/xmlconfig.o ../../common/driverfuncs.o ../common/texmem.o ../common/drirenderbuffer.o ../common/dri_metaops.o i830_context.o i830_state.o i830_texblend.o i830_texstate.o i830_vtbl.o intel_render.o intel_regions.o intel_buffer_objects.o intel_batchbuffer.o intel_clear.o intel_extensions.o intel_mipmap_tree.o intel_tex_layout.o intel_tex_image.o intel_tex_subimage.o intel_tex_copy.o intel_tex_validate.o intel_tex_format.o intel_tex.o intel_pixel.o intel_pixel_bitmap.o intel_pixel_copy.o intel_pixel_draw.o intel_pixel_read.o intel_buffers.o intel_blit.o i915_tex_layout.o i915_texstate.o i915_context.o i915_debug.o i915_debug_fp.o i915_fragprog.o i915_program.o i915_state.o i915_vtbl.o intel_context.o intel_decode.o intel_screen.o intel_span.o intel_state.o intel_syncobj.o intel_tris.o intel_fbo.o   ../../../../../src/mesa/libmesa.a    -L/opt/gfx-test/lib -ldrm   -lexpat -lm -lpthread -ldl -L/opt/gfx-test/lib -ldrm_intel -ldrm  
mklib: Making Linux shared library:  i915_dri.so.tmp
gcc -o i915_dri.so.test ../../../../../src/mesa/drivers/dri/common/dri_test.o i915_dri.so.tmp -L../../../../../lib -lGL
i915_dri.so.tmp: undefined reference to `drm_intel_bo_emit_reloc_fence'
i915_dri.so.tmp: undefined reference to `drm_intel_bufmgr_gem_enable_fenced_relocs'
collect2: ld returned 1 exit status
gmake[6]: *** [i915_dri.so] Error 1

Looks like the mklib call has the right flags, but the test link
doesn't, so it tries to link against a libdrm w/o these functions
exported.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Xavier C. <cha...@gm...>]

On Fri, Mar 19, 2010 at 10:39 PM, Dan Nicholson <dbn...@gm...> wrote:
> On Fri, Mar 19, 2010 at 2:19 PM, Luca Barbieri <lu...@lu...> wrote:
>>> Can we just put this program in the demos? Or at least just make it a
>>> separate target (make test-link)? It seems excessive to make this part
>>> of the default build path.
>>
>> The whole purpose is to run this as part of the standard build, so
>> that the build fails if any driver is unloadable, (i.e. a modification
>> to it was botched) and the tree hopefully doesn't get pushed to
>> master.
>>
>> You can test it separately by just running glxinfo/glxgears, obviously.
>
> For developers that makes a lot of sense, but I've never seen any
> other projects impose this type of thing on regular users.
>

You should only count the projects that allow by default successful
build with undefined symbols (I have honestly no idea how common that
is).
This seems especially bad in mesa where there are many people working
on core parts affecting all drivers and who cannot test easily all the
drivers. At least a build failure to catch the obvious mistakes would
be nice, though that will clearly not solve everything.

And I think it is as profitable to the user, who do not use / know
about LIBGL_DEBUG=verbose , and won't spot easily the dri failure and
the fallback to software rendering.

Re: [Mesa3d-dev] [PATCH] dri: test whether the built drivers have unresolved symbols

[Brian P. <bri...@gm...>]

On Fri, Mar 19, 2010 at 4:17 PM, Xavier Chantry
<cha...@gm...> wrote:
> On Fri, Mar 19, 2010 at 10:39 PM, Dan Nicholson <dbn...@gm...> wrote:
>> On Fri, Mar 19, 2010 at 2:19 PM, Luca Barbieri <lu...@lu...> wrote:
>>>> Can we just put this program in the demos? Or at least just make it a
>>>> separate target (make test-link)? It seems excessive to make this part
>>>> of the default build path.
>>>
>>> The whole purpose is to run this as part of the standard build, so
>>> that the build fails if any driver is unloadable, (i.e. a modification
>>> to it was botched) and the tree hopefully doesn't get pushed to
>>> master.
>>>
>>> You can test it separately by just running glxinfo/glxgears, obviously.
>>
>> For developers that makes a lot of sense, but I've never seen any
>> other projects impose this type of thing on regular users.
>>
>
> You should only count the projects that allow by default successful
> build with undefined symbols (I have honestly no idea how common that
> is).
> This seems especially bad in mesa where there are many people working
> on core parts affecting all drivers and who cannot test easily all the
> drivers. At least a build failure to catch the obvious mistakes would
> be nice, though that will clearly not solve everything.
>
> And I think it is as profitable to the user, who do not use / know
> about LIBGL_DEBUG=verbose , and won't spot easily the dri failure and
> the fallback to software rendering.


I'm OK with this change.  If it causes hardship for anyone it could
easily be reverted and revisited.

-Brian