Bug 91106 - glx: make check fails to build on osx
Summary: glx: make check fails to build on osx
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other Mac OS X (All)
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-25 08:18 UTC by Julien Isorce
Modified: 2018-05-09 08:28 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
glx: fix unit tests build on osx (4.30 KB, text/plain)
2015-06-25 08:18 UTC, Julien Isorce
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Isorce 2015-06-25 08:18:06 UTC
Created attachment 116709 [details]
glx: fix unit tests build on osx

It fails because of missing symbols and some duplicated symbols.
    
The attached patch adds missing dependencies in tests/Makefile.am to fix some missing symbols.

The attached patch also marks some functions with attribute weak to fix some duplicated symbols due to redefinitions in the unit tests.

It allows to fix the build.

Then it passes some tests but crash around the 30th test (glXCreateContextAttribARB_test.does_send_protocol):

It seems to crash when calling apple_cgl.choose_pixel_format. I investigated a bit and glx/apple/apple_cgl.c::apple_cgl_init is not called (so apple_cgl.choose_pixel_format = sym(h, "CGLChoosePixelFormat"); is not called).

backtrace with lldb:

* thread #1: tid = 0x86cd39, 0x0000000000000000, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000000000000
(lldb) bt
* thread #1: tid = 0x86cd39, 0x0000000000000000, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000000000000
    frame #1: 0x00000001000879fa glx-test`apple_visual_create_pfobj(pfobj=0x0000000100c22e88, mode=0x0000000100c22328, double_buffered=0x0000000100c22ea4, uses_stereo=0x0000000100c22ea5, offscreen=false) + 1610 at apple_visual.c:169
    frame #2: 0x00000001000833b9 glx-test`apple_glx_create_context(ptr=0x0000000100c22de8, dpy=0x0000000000000000, screen=0, mode=0x0000000100c22328, sharedContext=0x0000000000000000, errorptr=0x00007fff5fbff09c, x11errorptr=0x00007fff5fbff09b) + 377 at apple_glx_context.c:161
    frame #3: 0x00000001000823ec glx-test`applegl_create_context(psc=0x0000000100c22c90, config=0x0000000100c22328, shareList=0x0000000000000000, renderType=0) + 332 at applegl_glx.c:150
    frame #4: 0x00000001000439f3 glx-test`glXCreateContextAttribsARB(dpy=0x000000010100e400, config=0x0000000100c22328, share_context=0x0000000000000000, direct=0, attrib_list=0x0000000000000000) + 451 at create_context.c:86
    frame #5: 0x00000001000155c1 glx-test`glXCreateContextAttribARB_test_does_send_protocol_Test::TestBody(this=0x0000000100c22310) + 97 at create_context_unittest.cpp:221
    frame #6: 0x00000001004a1f13 glx-test`void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(object=0x0000000100c22310, method=0x0000000000000021, location=0x000000010057f3fd)(), char const*) + 131 at gtest.cc:2078
    frame #7: 0x000000010048c7b7 glx-test`void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(object=0x0000000100c22310, method=0x0000000000000021, location=0x000000010057f3fd)(), char const*) + 119 at gtest.cc:2114
    frame #8: 0x0000000100465b05 glx-test`testing::Test::Run(this=0x0000000100c22310) + 197 at gtest.cc:2150
    frame #9: 0x0000000100466ddb glx-test`testing::TestInfo::Run(this=0x0000000100c1a420) + 219 at gtest.cc:2326
    frame #10: 0x0000000100467d07 glx-test`testing::TestCase::Run(this=0x0000000100c1a040) + 231 at gtest.cc:2444
    frame #11: 0x00000001004747f8 glx-test`testing::internal::UnitTestImpl::RunAllTests(this=0x0000000100c17590) + 952 at gtest.cc:4315
    frame #12: 0x000000010049ed93 glx-test`bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x0000000100c17590, method=0x0000000100474440, location=0x000000010057fabc)(), char const*) + 131 at gtest.cc:2078
    frame #13: 0x000000010048efc7 glx-test`bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x0000000100c17590, method=0x0000000100474440, location=0x000000010057fabc)(), char const*) + 119 at gtest.cc:2114
    frame #14: 0x00000001004743b6 glx-test`testing::UnitTest::Run(this=0x00000001005b3458) + 422 at gtest.cc:3926
    frame #15: 0x00000001004ac491 glx-test`RUN_ALL_TESTS() + 17 at gtest.h:2288
    frame #16: 0x00000001004ac46c glx-test`main(argc=1, argv=0x00007fff5fbffac0) + 60 at gtest_main.cc:37
    frame #17: 0x00007fff886ee5c9 libdyld.dylib`start + 1

Also I tried to manually call __glXInitialize (because it is never called) in tests/create_context_unittest.cpp::GetGLXScreenConfigs existing redefinition:

 GetGLXScreenConfigs(Display * dpy, int scrn)
 {
+   if(psc)
+     __glXInitialize(dpy);

But it still does not make "apple_cgl_init" to be called, because it crashes in glxext.c:847:   dpyPriv->codes = XInitExtension(dpy, __glXExtensionName);

* thread #1: tid = 0x86de8e, 0x0000000100989057 libX11.6.dylib`require_socket + 32, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
    frame #0: 0x0000000100989057 libX11.6.dylib`require_socket + 32
libX11.6.dylib`require_socket + 32:
-> 0x100989057:  cmpl   $0x0, 0x40(%rax)
   0x10098905b:  setne  %cl
   0x10098905e:  movzbl %cl, %ecx
   0x100989061:  movq   (%rax), %rdi
(lldb) bt
* thread #1: tid = 0x86de8e, 0x0000000100989057 libX11.6.dylib`require_socket + 32, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
  * frame #0: 0x0000000100989057 libX11.6.dylib`require_socket + 32
    frame #1: 0x0000000100989018 libX11.6.dylib`_XFlush + 14
    frame #2: 0x000000010098a1fb libX11.6.dylib`_XGetRequest + 53
    frame #3: 0x000000010098036f libX11.6.dylib`XQueryExtension + 68
    frame #4: 0x0000000100975a80 libX11.6.dylib`XInitExtension + 36
    frame #5: 0x0000000100048fc4 glx-test`__glXInitialize(dpy=0x0000000101803c00) + 308 at glxext.c:847
    frame #6: 0x000000010001490b glx-test`GetGLXScreenConfigs(dpy=0x0000000101803c00, scrn=0) + 75 at create_context_unittest.cpp:70
    frame #7: 0x00000001000438bd glx-test`glXCreateContextAttribsARB(dpy=0x0000000101803c00, config=0x0000000100d00098, share_context=0x0000000000000000, direct=0, attrib_list=0x0000000000000000) + 141 at create_context.c:59
    frame #8: 0x00000001000155c1 glx-test`glXCreateContextAttribARB_test_does_send_protocol_Test::TestBody(this=0x0000000100d00080) + 97 at create_context_unittest.cpp:221
    frame #9: 0x00000001004a1f13 glx-test`void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(object=0x0000000100d00080, method=0x0000000000000021, location=0x000000010057f3fd)(), char const*) + 131 at gtest.cc:2078
    frame #10: 0x000000010048c7b7 glx-test`void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(object=0x0000000100d00080, method=0x0000000000000021, location=0x000000010057f3fd)(), char const*) + 119 at gtest.cc:2114
    frame #11: 0x0000000100465b05 glx-test`testing::Test::Run(this=0x0000000100d00080) + 197 at gtest.cc:2150
    frame #12: 0x0000000100466ddb glx-test`testing::TestInfo::Run(this=0x0000000100c1a420) + 219 at gtest.cc:2326
    frame #13: 0x0000000100467d07 glx-test`testing::TestCase::Run(this=0x0000000100c1a040) + 231 at gtest.cc:2444
    frame #14: 0x00000001004747f8 glx-test`testing::internal::UnitTestImpl::RunAllTests(this=0x0000000100c17590) + 952 at gtest.cc:4315
    frame #15: 0x000000010049ed93 glx-test`bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x0000000100c17590, method=0x0000000100474440, location=0x000000010057fabc)(), char const*) + 131 at gtest.cc:2078
    frame #16: 0x000000010048efc7 glx-test`bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x0000000100c17590, method=0x0000000100474440, location=0x000000010057fabc)(), char const*) + 119 at gtest.cc:2114
    frame #17: 0x00000001004743b6 glx-test`testing::UnitTest::Run(this=0x00000001005b3458) + 422 at gtest.cc:3926
    frame #18: 0x00000001004ac491 glx-test`RUN_ALL_TESTS() + 17 at gtest.h:2288
    frame #19: 0x00000001004ac46c glx-test`main(argc=1, argv=0x00007fff5fbffac0) + 60 at gtest_main.cc:37
    frame #20: 0x00007fff886ee5c9 libdyld.dylib`start + 1
(lldb) 

Any comment about the patch to fix the build and about the crashes ?
Comment 1 Emil Velikov 2015-06-26 12:35:34 UTC
Hi Julien, I'm not sure how many people go bugzilla hunting for patches. Here are some things that stuck out:

From 2b424445a8c9028e72e2370da8dbdc969bb8717b Mon Sep 17 00:00:00 2001
From: Julien Isorce <j.isorce@samsung.com>
Date: Thu, 25 Jun 2015 08:59:27 +0100
Subject: [PATCH] glx: fix unit tests build on osx

It fails because of missing symbols
and some duplicated symbols.

The patch adds missing dependencies in tests/Makefile.am
to fix some missing symbols.
The patch also marks some functions with attribute weak
to fix some duplicated symbols due to redefinitions in
the unit tests.

Signed-off-by: Julien Isorce <j.isorce@samsung.com>
---
 src/glx/glxcmds.c         | 10 ++++++++--
 src/glx/glxcurrent.c      |  8 +++++++-
 src/glx/glxextensions.c   |  7 ++++++-
 src/glx/indirect_init.h   |  2 ++
 src/glx/tests/Makefile.am |  9 ++++++++-
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index 26ff804..f073ecb 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -55,6 +55,12 @@
 #include <xcb/xcb.h>
 #include <xcb/glx.h>
 
+#if defined(GLX_USE_APPLEGL)
+#define _ATTRIBUTE_WEAK __attribute__((weak))
+#else
+#define _ATTRIBUTE_WEAK
+#endif
+
(If we end up keeping this) Can we avoid redefining it X times ? Ian (the original author of these tests) should know the details as to why/how we don't end up with duplicated/conflicting symbols.


--- a/src/glx/tests/Makefile.am
+++ b/src/glx/tests/Makefile.am
@@ -21,7 +21,6 @@ glx_test_SOURCES =			\
 	enum_sizes.cpp			\
 	fake_glx_screen.cpp		\
 	fake_glx_screen.h		\
-	indirect_api.cpp		\
 	mock_xdisplay.h			\
 	query_renderer_unittest.cpp
 
@@ -30,9 +29,17 @@ glx_test_SOURCES += \
 	query_renderer_implementation_unittest.cpp
 endif
 
+if !HAVE_APPLEDRI
+glx_test_SOURCES += indirect_api.cpp
+endif
+
 glx_test_LDADD = \
 	$(top_builddir)/src/glx/libglx.la \
 	$(top_builddir)/src/gtest/libgtest.la \
+	$(top_builddir)/src/mapi/glapi/libglapi.la \
Which symbols are missing if we omit this ? Noone else seems to need it.

 	$(top_builddir)/src/mapi/shared-glapi/libglapi.la \
+	$(SHARED_GLAPI_LIB) \
Unset/empty variable.

+	$(GL_LIB_DEPS) \
Good catch, I think we can now nuke PTHEADS_LIBS below. Perhaps do this as a separate commit ?

+	$(X11_LIBS) \
Unset/empty variable. Upon closer look one can do a X11_* variable cleanup though mesa.

 	$(PTHREAD_LIBS)
 endif
-- 
1.9.5 (Apple Git-50.3)
Comment 2 Emil Velikov 2015-06-26 12:37:24 UTC
Although I would honestly suggest that one takes a closer look into src/glx{,/apple} and give it some much needed love. Iirc Jon had some branches which rework/clean things up which seems like the way forward imho.
Comment 3 Jon Turney 2015-06-27 13:18:55 UTC
(In reply to Emil Velikov from comment #1) 
> +#if defined(GLX_USE_APPLEGL)
> +#define _ATTRIBUTE_WEAK __attribute__((weak))
> +#else
> +#define _ATTRIBUTE_WEAK
> +#endif
> +
> (If we end up keeping this) Can we avoid redefining it X times ? Ian (the
> original author of these tests) should know the details as to why/how we
> don't end up with duplicated/conflicting symbols.

These really are duplicates that the test harness provides to override the
real versions.

Unfortunately, ld on OSX doesn't behave that way, and the option -multiply_defined suppress seems to be obsolete.

I'm not very keen on scattering __attribute__(weak) around to fix this, but I guess the only other approach is to split those symbols out into a separate object, which is linked with to produce libglx.a, but not into a convenience library which is used by the tests.

> +	$(top_builddir)/src/mapi/glapi/libglapi.la \
> Which symbols are missing if we omit this ? Noone else seems to need it.

glapi_create_table_from_handle, which only exists on APPLE, if memory serves.

But linking with glapi and shared-glapi doesn't seem right, so I think this is a sign that something else is wrong.
Comment 4 Jon Turney 2015-06-27 13:20:14 UTC
(In reply to Emil Velikov from comment #2)
> Iirc Jon had some
> branches which rework/clean things up which seems like the way forward imho.

Sorry, all the clean-up I did for __APPLE__ is already upstream, with the exception of the 2 patches which clearly need to be done in a better way.
Comment 5 Timothy Arceri 2018-05-09 05:48:05 UTC
There have been verious osx fixes since this bug was filed. Is this still a problem or can we close this bug?
Comment 6 Jeremy Huddleston Sequoia 2018-05-09 08:28:35 UTC
Pretty sure we can call this fixed with Jon's changes from a few years back.  If there are still issues, we'll need newer reports.


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.