Bug 41639

Summary:	__sync synchronisation primitives take unsigned as input and output values.
Product:	gcc	Reporter:	chrbr
Component:	middle-end	Assignee:	Not yet assigned to anyone <unassigned>
Status:	RESOLVED INVALID
Severity:	trivial	CC:	gcc-bugs, rth
Priority:	P3	Keywords:	wrong-code
Version:	4.5.0
Target Milestone:	---
Host:		Target:	sh-
Build:		Known to work:
Known to fail:		Last reconfirmed:
Attachments:	Fix synchronisation parameter/output signess

Description chrbr 2009-10-09 07:06:16 UTC

Current implementation of the synchronization builtins in gcc (from http://software.intel.com/sites/products/documentation/hpc/compilerpro/en-us/cpp/lin/compiler_c/intref_cls/common/intref_itanium_synchro_prim.htm) describe <type> as unsigned. although it is stated as "<type> is either a 32-bit or 64-bit integer"

consequently, testsuite tests such as sync-2.c:

if (__sync_sub_and_fetch(AI+13, 12) != (char)-12)
    abort ();

might fail. (unless the target/runtime dependant primitive implementation artificially change the return type).

Comment 1 chrbr 2009-10-09 07:12:40 UTC

Created attachment 18758 [details]
Fix synchronisation parameter/output signess

The attached patch gives the correct semantic. But should be checked on target using them (pa/arm) for possible legacy regression.

(tested on SH with a non-linux, in house runtime, implementation)

2009-10-08  Christian Bruel  <christian.bruel@st.com>
 
	* builtin-types.def (BT_I[1,2,4,8,16): Set signed.

Comment 2 Steven Bosscher 2010-07-20 22:21:38 UTC

Mis-categorized as a treelang bug (?!).

rth, you added these primitive types and the _sync_* builtins... Could you have a look at this bug and at the patch of comment #1, please?

Comment 3 Andrew Pinski 2024-03-11 00:34:39 UTC

So I looked into the history here and the code was correct even when this bug was fixed.

Basically resolve_overloaded_builtin will resolve __sync_sub_and_fetch into
__sync_sub_and_fetch_1 and add a cast for the return value to the same type as the first argument this is done in sync_resolve_return.


48ae6c138ca3 (Richard Henderson         2005-04-14 16:37:47 -0700 8801)         result = build_function_call (new_function, params);
48ae6c138ca3 (Richard Henderson         2005-04-14 16:37:47 -0700 8802)         if (orig_code != BUILT_IN_BOOL_COMPARE_AND_SWAP_N
48ae6c138ca3 (Richard Henderson         2005-04-14 16:37:47 -0700 8803)             && orig_code != BUILT_IN_LOCK_RELEASE_N)
48ae6c138ca3 (Richard Henderson         2005-04-14 16:37:47 -0700 8804)           result = sync_resolve_return (params, result);


So unless there is something wrong with the way sh implements these functions in libgcc there is no issue in the middle-end.

Comment 4 Andrew Pinski 2024-03-11 00:39:50 UTC

>(tested on SH with a non-linux, in house runtime, implementation)


The bug is in your runtime implementation I think.

Which has a mismatch in the return type vs what GCC internals think these should be.

Note libgcc/sync.c does:
```
#if SIZE == 1 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1

typedef unsigned int UQItype __attribute__((mode (QI)));
DEFINE (FN, 1, UQItype)

#elif SIZE == 2 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2

typedef unsigned int UHItype __attribute__((mode (HI)));
DEFINE (FN, 2, UHItype)

#elif SIZE == 4 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4

typedef unsigned int USItype __attribute__((mode (SI)));
DEFINE (FN, 4, USItype)

#elif SIZE == 8 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8

typedef unsigned int UDItype __attribute__((mode (DI)));
DEFINE (FN, 8, UDItype)

#elif SIZE == 16 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16

typedef unsigned int UOItype __attribute__((mode (OI)));
DEFINE (FN, 8, UOItype)


```

Which matches the internals of GCC.