Bug 93089 - mesa fails to check for gcc atomic primitives before using them
Summary: mesa fails to check for gcc atomic primitives before using them
Status: REOPENED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-24 01:38 UTC by Jonathan Gray
Modified: 2017-04-05 00:52 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
i486 __sync_val_compare_and_swap_8 disable debug message (722 bytes, patch)
2016-09-29 13:14 UTC, Elmar Hanlhofer
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Gray 2015-11-24 01:38:53 UTC
The u_atomic.h macros assume the gcc atomic builtins are always available but this is only the case on linux with the way the linux kernel fakes atomics when the architecture does not provide them.

This concerns at least sparc < v9, hppa, sh, arm < v6, alpha and mips64.

mips64
undefined reference to `__sync_val_compare_and_swap_1'

alpha
undefined reference to `__sync_val_compare_and_swap_1'

i386 (should be avoidable with -march=i586 for cx8)
undefined symbol '__sync_val_compare_and_swap_8'

arm v4 abi
/tmp//ccIl4d4c.o: In function `test_atomic_assign_int':
u_atomic_test.c:(.text+0x78): undefined reference to `__sync_val_compare_and_swap_4'
u_atomic_test.c:(.text+0xd4): undefined reference to `__sync_val_compare_and_swap_4'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_int':
u_atomic_test.c:(.text+0x170): undefined reference to `__sync_add_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_int':
u_atomic_test.c:(.text+0x1dc): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x240): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x2ac): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x318): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x350): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x3b4): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x3ec): undefined reference to `__sync_sub_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_unsigned':
u_atomic_test.c:(.text+0x4f4): undefined reference to `__sync_val_compare_and_swap_4'
u_atomic_test.c:(.text+0x550): undefined reference to `__sync_val_compare_and_swap_4'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_unsigned':
u_atomic_test.c:(.text+0x5ec): undefined reference to `__sync_add_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_unsigned':
u_atomic_test.c:(.text+0x658): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x6bc): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x728): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x794): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x7cc): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x830): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x868): undefined reference to `__sync_sub_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_int16_t':
u_atomic_test.c:(.text+0x978): undefined reference to `__sync_val_compare_and_swap_2'
u_atomic_test.c:(.text+0x9dc): undefined reference to `__sync_val_compare_and_swap_2'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_int16_t':
u_atomic_test.c:(.text+0xa7c): undefined reference to `__sync_add_and_fetch_2'
/tmp//ccIl4d4c.o: In function `test_atomic_int16_t':
u_atomic_test.c:(.text+0xae8): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0xb54): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0xbc0): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0xc34): undefined reference to `__sync_add_and_fetch_2'
u_atomic_test.c:(.text+0xc6c): undefined reference to `__sync_add_and_fetch_2'
u_atomic_test.c:(.text+0xcd8): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0xd18): undefined reference to `__sync_sub_and_fetch_2'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_uint16_t':
u_atomic_test.c:(.text+0xe38): undefined reference to `__sync_val_compare_and_swap_2'
u_atomic_test.c:(.text+0xe9c): undefined reference to `__sync_val_compare_and_swap_2'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_uint16_t':
u_atomic_test.c:(.text+0xf40): undefined reference to `__sync_add_and_fetch_2'
/tmp//ccIl4d4c.o: In function `test_atomic_uint16_t':
u_atomic_test.c:(.text+0xfac): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0x1010): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0x107c): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0x10ec): undefined reference to `__sync_add_and_fetch_2'
u_atomic_test.c:(.text+0x1124): undefined reference to `__sync_add_and_fetch_2'
u_atomic_test.c:(.text+0x1188): undefined reference to `__sync_sub_and_fetch_2'
u_atomic_test.c:(.text+0x11c4): undefined reference to `__sync_sub_and_fetch_2'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_int32_t':
u_atomic_test.c:(.text+0x12d4): undefined reference to `__sync_val_compare_and_swap_4'
u_atomic_test.c:(.text+0x1330): undefined reference to `__sync_val_compare_and_swap_4'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_int32_t':
u_atomic_test.c:(.text+0x13cc): undefined reference to `__sync_add_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_int32_t':
u_atomic_test.c:(.text+0x1438): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x149c): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x1508): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x1574): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x15ac): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x1610): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x1648): undefined reference to `__sync_sub_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_uint32_t':
u_atomic_test.c:(.text+0x1750): undefined reference to `__sync_val_compare_and_swap_4'
u_atomic_test.c:(.text+0x17ac): undefined reference to `__sync_val_compare_and_swap_4'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_uint32_t':
u_atomic_test.c:(.text+0x1844): undefined reference to `__sync_add_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_uint32_t':
u_atomic_test.c:(.text+0x18ac): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x1910): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x197c): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x19e8): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x1a20): undefined reference to `__sync_add_and_fetch_4'
u_atomic_test.c:(.text+0x1a84): undefined reference to `__sync_sub_and_fetch_4'
u_atomic_test.c:(.text+0x1abc): undefined reference to `__sync_sub_and_fetch_4'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_int64_t':
u_atomic_test.c:(.text+0x1c14): undefined reference to `__sync_val_compare_and_swap_8'
u_atomic_test.c:(.text+0x1cb4): undefined reference to `__sync_val_compare_and_swap_8'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_int64_t':
u_atomic_test.c:(.text+0x1d80): undefined reference to `__sync_add_and_fetch_8'
/tmp//ccIl4d4c.o: In function `test_atomic_int64_t':
u_atomic_test.c:(.text+0x1e10): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x1eb4): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x1f4c): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x2000): undefined reference to `__sync_add_and_fetch_8'
u_atomic_test.c:(.text+0x2050): undefined reference to `__sync_add_and_fetch_8'
u_atomic_test.c:(.text+0x20f8): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x215c): undefined reference to `__sync_sub_and_fetch_8'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_uint64_t':
u_atomic_test.c:(.text+0x2304): undefined reference to `__sync_val_compare_and_swap_8'
u_atomic_test.c:(.text+0x23a4): undefined reference to `__sync_val_compare_and_swap_8'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_uint64_t':
u_atomic_test.c:(.text+0x2470): undefined reference to `__sync_add_and_fetch_8'
/tmp//ccIl4d4c.o: In function `test_atomic_uint64_t':
u_atomic_test.c:(.text+0x2500): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x25a4): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x263c): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x26f0): undefined reference to `__sync_add_and_fetch_8'
u_atomic_test.c:(.text+0x2740): undefined reference to `__sync_add_and_fetch_8'
u_atomic_test.c:(.text+0x27e8): undefined reference to `__sync_sub_and_fetch_8'
u_atomic_test.c:(.text+0x284c): undefined reference to `__sync_sub_and_fetch_8'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_int8_t':
u_atomic_test.c:(.text+0x29a8): undefined reference to `__sync_val_compare_and_swap_1'
u_atomic_test.c:(.text+0x2a0c): undefined reference to `__sync_val_compare_and_swap_1'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_int8_t':
u_atomic_test.c:(.text+0x2aa4): undefined reference to `__sync_add_and_fetch_1'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_uint8_t':
u_atomic_test.c:(.text+0x2b5c): undefined reference to `__sync_val_compare_and_swap_1'
u_atomic_test.c:(.text+0x2bb8): undefined reference to `__sync_val_compare_and_swap_1'
/tmp//ccIl4d4c.o: In function `test_atomic_8bits_uint8_t':
u_atomic_test.c:(.text+0x2c54): undefined reference to `__sync_add_and_fetch_1'
/tmp//ccIl4d4c.o: In function `test_atomic_assign_bool':
u_atomic_test.c:(.text+0x2d20): undefined reference to `__sync_val_compare_and_swap_1'
u_atomic_test.c:(.text+0x2d98): undefined reference to `__sync_val_compare_and_swap_1'
collect2: ld returned 1 exit status
Comment 1 Ian Romanick 2015-11-30 23:37:23 UTC
Matt: I know you're an Alpha fan.  Suggestions?
Comment 2 Jonathan Gray 2015-12-01 01:11:21 UTC
It seems mips64 got the sync builtins later than other platforms, they are available in later versions of gcc but not gcc 4.2.

Alpha also hit an ICE so for now we stopped building Mesa on alpha/mips64 in OpenBSD.

lib/mesa/src/mesa/main/format_pack.c  -fPIC -DPIC -o main/.libs/format_pack.o                                                
/usr/xenocara/lib/mesa/src/mesa/main/format_pack.c: In function 'pack_float_bgr_srgb8':                                      
/usr/xenocara/lib/mesa/src/mesa/main/format_pack.c:6890: error: unrecognizable insn:                                         
(insn 259 11 13 2 (set (reg:SF 1 $1)                                                                                         
        (const_double:SF 1.220703125e-4 [0x0.8p-12])) -1 (nil)                                                               
    (nil))                                                                                                                   
/usr/xenocara/lib/mesa/src/mesa/main/format_pack.c:6890: internal compiler error: in extract_insn, at recog.c:2077

For other architectures we currently build with:

#elif defined(__ARM_ARCH_4__) || defined(__ARM_ARCH_5__) || \
      defined(__hppa__) || defined(__sparc__) || defined(__sh__)
#define PIPE_ATOMIC_UNLOCKED

...

#if defined(PIPE_ATOMIC_UNLOCKED)

#define PIPE_ATOMIC "Unlocked"

#define p_atomic_set(_v, _i) (*(_v) = (_i))
#define p_atomic_read(_v) (*(_v))
#define p_atomic_dec_zero(_v) ((*(_v) -= 1) == 0)
#define p_atomic_inc(_v) (*(_v) += 1)
#define p_atomic_dec(_v) (*(_v) -= 1)
#define p_atomic_add(_v, _i) (*(_v) += (_i))
#define p_atomic_inc_return(_v) (*(_v) += 1)
#define p_atomic_dec_return(_v) (*(_v) -= 1)
#define p_atomic_cmpxchg(_v, old, _new) ({	\
	__typeof(*_v) _r = *(_v);		\
	if (*(_v) == old)			\
		*(_v) = (_new);			\
	_r;					\
})
#endif
Comment 3 Jose Fonseca 2015-12-02 11:18:16 UTC
Does OpenBSD supply stdatomic.h?

I wonder if we should start using it on all platforms that supply it.
Comment 4 Jonathan Gray 2015-12-02 11:50:25 UTC
No.  And it seems gcc only started supporting C11 atomics in 4.9.

libdrm currently checks for gcc __sync builtins, libatomic-ops and Solaris atomic.h
Comment 5 Matt Turner 2015-12-02 18:04:01 UTC
I think if OpenBSD insists on compiling current software with an 8 year old compiler, they should expect to implement some widely available features in their fork...
Comment 6 Jonathan Gray 2015-12-03 01:04:06 UTC
I added support for the gnu binary integer constant extension to gcc specifically for the intel driver in Mesa.  We've also changed the behaviour of the sync builtins with PIC on i386 to not clobber ebx as well and people are working on implementing the sync builtins for mips64.  Mesa won't build with newer versions of out of tree compilers anyway due to the problem misdetecting SSE4.1 there is another PR for (https://bugs.freedesktop.org/show_bug.cgi?id=91806).

Even with the latest GCC these atomic builtins won't be available on all platforms anyway.  Mesa is rapidly becoming i386/amd64/powerpc/sparc64 only.
Comment 7 Emil Velikov 2015-12-04 16:49:49 UTC
What Matt is saying is that surely Mesa is not the only project out there which relies on atomics, thus adding (backporting) those in GCC wouldn't be that bad of an idea.

That aside I'm thinking that porting the libdrm solution wouldn't be that bad of an idea.
Comment 8 Jonathan Gray 2015-12-04 17:29:37 UTC
There is no way to backport builtins for architectures that lack atomics at the ISA level.

On arm/hppa/sh linux has a syscall which the gcc documentation claims can only do 64 bit values on arm:
https://gcc.gnu.org/wiki/Atomic

Even on Linux with the latest toolchains this should be checked as there are cases where some or none of the atomics may be present.
Comment 9 Lauri Kasanen 2016-08-03 17:55:10 UTC
We're hitting this on TinyCore, and I see Alpine hit it a while back too [1].

We build with -march=i486, and some of the atomics require a 586. Things build, but some drivers fail to load:

libGL: dlopen /usr/local/lib/dri/i915_dri.so failed (/usr/local/lib/dri/i915_dri.so: undefined symbol: __sync_val_compare_and_swap_8)
libGL error: unable to load driver: i915_dri.so
libGL error: driver pointer missing

They should fail to build at the very least. Changing the hw and os to all.

[1] https://bugs.alpinelinux.org/issues/4254
Comment 10 Elmar Hanlhofer 2016-09-29 13:14:33 UTC
Created attachment 126862 [details] [review]
i486 __sync_val_compare_and_swap_8 disable debug message

I came across the same problem when I compiled Mesa for the new Plop Linux i486 release. As the affected code is only in a debug message routine in "intel_debug.c", I simply disabled it. Now it compiles fine with "march=i486". I attached the patch.
Comment 11 Jonathan Gray 2017-01-21 05:06:54 UTC
Previously 64 bit atomics were contained to i965.  The on disk shader cache introduced in Mesa 13.0 in 87ab26b2ab35a29d446ae66f1795d40c184c0739 now requires 64 bit atomics as well which breaks Mesa on 32 bit powerpc which doesn't have 64 bit atomic instructions:

glxinfo:/usr/X11R6/lib/modules/dri/r300_dri.so: undefined symbol '__sync_add_and_fetch_8'
libGL error: unable to load driver: r300_dri.so
libGL error: driver pointer missing
libGL error: failed to load driver: r300
glxinfo:/usr/X11R6/lib/modules/dri/swrast_dri.so: undefined symbol '__sync_add_and_fetch_8'
libGL error: unable to load driver: swrast_dri.so
libGL error: failed to load driver: swrast
X Error of failed request:  GLXBadContext
  Major opcode of failed request:  155 (GLX)
  Minor opcode of failed request:  6 (X_GLXIsDirect)
  Serial number of failed request:  47
  Current serial number in output stream:  0
name of display: :0

This was reported by Stefan Sperling on OpenBSD/macppc along with the workaround of using --disable-shader-cache.
Comment 12 Grazvydas Ignotas 2017-04-04 22:19:44 UTC
Fixes (partial at least) pushed as a6a38a038bd62e6d9558905f00bef81b5e7e6fcc and b384c23b9e804916f125354e06c735bd3bb22811. Please reopen if there is still a problem and specify the environment (architecture, compiler version, any extra flags).
Comment 13 Jonathan Gray 2017-04-04 22:28:15 UTC
This is still not handled for 32 bit atomics.
Comment 14 Grazvydas Ignotas 2017-04-04 23:29:22 UTC
Yes but any affected platforms we actually care about?

Newer gcc should cover some of the cases mentioned in this bug. I think pre-v6 ARM now has 32bit atomics on Linux thanks to a kernel helper.
Others you mentioned are hppa and superh, is anyone actually building mesa for those?

Otherwise it will just be wasted effort for some dead code that nobody ever uses.
Comment 15 Jonathan Gray 2017-04-04 23:33:36 UTC
Shocking as it may seem Mesa runs on more than just Linux.
Comment 16 Matt Turner 2017-04-04 23:40:28 UTC
(In reply to Jonathan Gray from comment #15)
> Shocking as it may seem Mesa runs on more than just Linux.

Don't act like that. Grazvydas wrote code to fix a bug you reported more than two years ago. You should be thankful, not sarcastic.

Presumably if you don't piss him off he'd be amenable to fixing other things in this area.
Comment 17 Matt Turner 2017-04-04 23:42:51 UTC
(In reply to Grazvydas Ignotas from comment #14)
> Yes but any affected platforms we actually care about?
> 
> Newer gcc should cover some of the cases mentioned in this bug. I think
> pre-v6 ARM now has 32bit atomics on Linux thanks to a kernel helper.
> Others you mentioned are hppa and superh, is anyone actually building mesa
> for those?
> 
> Otherwise it will just be wasted effort for some dead code that nobody ever
> uses.

Gentoo supports both hppa and sh. I find it doubtful that anyone would use Mesa on sh, but hppa is well maintained. I should have a system running in a few days on which I can test.

You figured out the solution -- I'm happy to copy-and-paste your small amount of code to support hppa.
Comment 18 Jonathan Gray 2017-04-04 23:53:40 UTC
(In reply to Matt Turner from comment #16)
> (In reply to Jonathan Gray from comment #15)
> > Shocking as it may seem Mesa runs on more than just Linux.
> 
> Don't act like that. Grazvydas wrote code to fix a bug you reported more
> than two years ago. You should be thankful, not sarcastic.
> 
> Presumably if you don't piss him off he'd be amenable to fixing other things
> in this area.

The problem was clearly stated in the original report.  The changes that went in have not addressed this.  Only a variant that concerns a subset.  Closing the bug because it fixed the case he cared about and not the actual reported problem isn't helping anyone.

I've had to workaround the breakage for years to be able to ship builds of Mesa on multiple architectures.
Comment 19 Timothy Arceri 2017-04-05 00:25:44 UTC
(In reply to Jonathan Gray from comment #18)
> (In reply to Matt Turner from comment #16)
> > (In reply to Jonathan Gray from comment #15)
> > > Shocking as it may seem Mesa runs on more than just Linux.
> > 
> > Don't act like that. Grazvydas wrote code to fix a bug you reported more
> > than two years ago. You should be thankful, not sarcastic.
> > 
> > Presumably if you don't piss him off he'd be amenable to fixing other things
> > in this area.
> 
> The problem was clearly stated in the original report.  The changes that
> went in have not addressed this.  Only a variant that concerns a subset. 
> Closing the bug because it fixed the case he cared about and not the actual
> reported problem isn't helping anyone.

I don't think he cared about it, he was trying to fix the bug for you.

> 
> I've had to workaround the breakage for years to be able to ship builds of
> Mesa on multiple architectures.

Shocking as it may seem Mesa development in mainly done on Linux for Linux on non-niche modern hardware. Occasionally things are going to break on systems untested during development, complaining about an inevitability is not very helpful.

This is a community project with limited resources, we will always need contributions from people interested in keeping such platforms building/running optimally.
Comment 20 Matt Turner 2017-04-05 00:52:37 UTC
(In reply to Jonathan Gray from comment #18)
> (In reply to Matt Turner from comment #16)
> > (In reply to Jonathan Gray from comment #15)
> > > Shocking as it may seem Mesa runs on more than just Linux.
> > 
> > Don't act like that. Grazvydas wrote code to fix a bug you reported more
> > than two years ago. You should be thankful, not sarcastic.
> > 
> > Presumably if you don't piss him off he'd be amenable to fixing other things
> > in this area.
> 
> The problem was clearly stated in the original report.  The changes that
> went in have not addressed this.  Only a variant that concerns a subset. 
> Closing the bug because it fixed the case he cared about and not the actual
> reported problem isn't helping anyone.
> 
> I've had to workaround the breakage for years to be able to ship builds of
> Mesa on multiple architectures.

I think you're inappropriately attributing malice to comments 12 and 14.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.