| Message ID | CABOHX+dTtGXn+8ukwhyzZCMye0CRdKGNtRzcfwQDBJL3d3rfhw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [netbsd] Define TARGET_D_CRITSEC_SIZE for D language | expand |
On 24.04.2019 01:13, Iain Buclaw wrote: > Hi, > > This patch adds missing implementation of TARGET_D_CRITSEC_SIZE, which > would be noticed when using any bare synchronized statements. > > I couldn't see any target-specific alternatives of pthread_mutex_t in > netbsd headers, so the condition should be right. > > OK for trunk? > This patch is wrong. sizeof(pthread_mutex_t) depends on CPU. Just check that __cpu_simple_lock_nv_t that can be char, int, struct.
On Wed, 24 Apr 2019 at 01:56, Kamil Rytarowski <n54@gmx.com> wrote: > > On 24.04.2019 01:13, Iain Buclaw wrote: > > Hi, > > > > This patch adds missing implementation of TARGET_D_CRITSEC_SIZE, which > > would be noticed when using any bare synchronized statements. > > > > I couldn't see any target-specific alternatives of pthread_mutex_t in > > netbsd headers, so the condition should be right. > > > > OK for trunk? > > > > This patch is wrong. > > sizeof(pthread_mutex_t) depends on CPU. > > Just check that __cpu_simple_lock_nv_t that can be char, int, struct. > Ah, thanks for pointing to that. I've made a small working example, and it looks like only three have a different size, aarch64, hppa, and hppa64. https://explore.dgnu.org/z/U29cni I'll add special handling for them, but otherwise 48/28 looks like the reasonable default.
On 24.04.2019 03:30, Iain Buclaw wrote: > On Wed, 24 Apr 2019 at 01:56, Kamil Rytarowski <n54@gmx.com> wrote: >> >> On 24.04.2019 01:13, Iain Buclaw wrote: >>> Hi, >>> >>> This patch adds missing implementation of TARGET_D_CRITSEC_SIZE, which >>> would be noticed when using any bare synchronized statements. >>> >>> I couldn't see any target-specific alternatives of pthread_mutex_t in >>> netbsd headers, so the condition should be right. >>> >>> OK for trunk? >>> >> >> This patch is wrong. >> >> sizeof(pthread_mutex_t) depends on CPU. >> >> Just check that __cpu_simple_lock_nv_t that can be char, int, struct. >> > > Ah, thanks for pointing to that. I've made a small working example, > and it looks like only three have a different size, aarch64, hppa, and > hppa64. > > https://explore.dgnu.org/z/U29cni > > I'll add special handling for them, but otherwise 48/28 looks like the > reasonable default. > I recommend to solve it differently: autodetect the size during ./configure. I'm not sure that it is still correct. The sizes can also change over time. There is coming refactoring of libpthread(3) in NetBSD that can change sizes of these types, at least on some platforms.
On Wed, 24 Apr 2019 at 13:03, Kamil Rytarowski <n54@gmx.com> wrote: > > On 24.04.2019 03:30, Iain Buclaw wrote: > > On Wed, 24 Apr 2019 at 01:56, Kamil Rytarowski <n54@gmx.com> wrote: > >> > >> On 24.04.2019 01:13, Iain Buclaw wrote: > >>> Hi, > >>> > >>> This patch adds missing implementation of TARGET_D_CRITSEC_SIZE, which > >>> would be noticed when using any bare synchronized statements. > >>> > >>> I couldn't see any target-specific alternatives of pthread_mutex_t in > >>> netbsd headers, so the condition should be right. > >>> > >>> OK for trunk? > >>> > >> > >> This patch is wrong. > >> > >> sizeof(pthread_mutex_t) depends on CPU. > >> > >> Just check that __cpu_simple_lock_nv_t that can be char, int, struct. > >> > > > > Ah, thanks for pointing to that. I've made a small working example, > > and it looks like only three have a different size, aarch64, hppa, and > > hppa64. > > > > https://explore.dgnu.org/z/U29cni > > > > I'll add special handling for them, but otherwise 48/28 looks like the > > reasonable default. > > > > I recommend to solve it differently: autodetect the size during > ./configure. I'm not sure that it is still correct. > That would not work when building crosses. > The sizes can also change over time. There is coming refactoring of > libpthread(3) in NetBSD that can change sizes of these types, at least > on some platforms. > Maybe explaining the intended use better might help. This is only for the following lowering done in the front-end (my upstream): synchronized { var = 0; } Loosely converted into equivalent C. static char __critsec64[48]; _d_criticalenter(& __critsec64); var = 0; _d_criticalexit(& __critsec64); So long as the statically allocated pthread_mutex_t is big enough, there should be no problems.
On 24.04.2019 13:25, Iain Buclaw wrote: > On Wed, 24 Apr 2019 at 13:03, Kamil Rytarowski <n54@gmx.com> wrote: >> >> On 24.04.2019 03:30, Iain Buclaw wrote: >>> On Wed, 24 Apr 2019 at 01:56, Kamil Rytarowski <n54@gmx.com> wrote: >>>> >>>> On 24.04.2019 01:13, Iain Buclaw wrote: >>> https://explore.dgnu.org/z/U29cni >>> >>> I'll add special handling for them, but otherwise 48/28 looks like the >>> reasonable default. >>> >> OK, so please go for this.
On Wed, 24 Apr 2019 at 15:28, Kamil Rytarowski <n54@gmx.com> wrote: > > On 24.04.2019 13:25, Iain Buclaw wrote: > > On Wed, 24 Apr 2019 at 13:03, Kamil Rytarowski <n54@gmx.com> wrote: > >> > >> On 24.04.2019 03:30, Iain Buclaw wrote: > >>> On Wed, 24 Apr 2019 at 01:56, Kamil Rytarowski <n54@gmx.com> wrote: > >>>> > >>>> On 24.04.2019 01:13, Iain Buclaw wrote: > >>> https://explore.dgnu.org/z/U29cni > >>> > >>> I'll add special handling for them, but otherwise 48/28 looks like the > >>> reasonable default. > >>> > >> > > OK, so please go for this. > On inspection, it looks like there's no current configuration handling of aarch64-netbsd, hppa-netbsd, nor hppa64-netbsd. I've updated the patch to add them, and I've tested with config-list.mk LIST="aarch64-netbsd hppa-netbsd hppa64-netbsd", however I don't think that the changes to config.gcc should be included unless it has been ported proper.
diff --git a/gcc/config/netbsd-d.c b/gcc/config/netbsd-d.c index 76342aacae3..c49366dc23b 100644 --- a/gcc/config/netbsd-d.c +++ b/gcc/config/netbsd-d.c @@ -28,6 +28,8 @@ along with GCC; see the file COPYING3. If not see #include "d/d-target.h" #include "d/d-target-def.h" +/* Implement TARGET_D_OS_VERSIONS for NetBSD targets. */ + static void netbsd_d_os_builtins (void) { @@ -35,7 +37,19 @@ netbsd_d_os_builtins (void) d_add_builtin_version ("NetBSD"); } +/* Implement TARGET_D_CRITSEC_SIZE for NetBSD targets. */ + +static unsigned +netbsd_d_critsec_size (void) +{ + /* This is the sizeof pthread_mutex_t. */ + return (POINTER_SIZE == 64) ? 48 : 28; +} + #undef TARGET_D_OS_VERSIONS #define TARGET_D_OS_VERSIONS netbsd_d_os_builtins +#undef TARGET_D_CRITSEC_SIZE +#define TARGET_D_CRITSEC_SIZE netbsd_d_critsec_size + struct gcc_targetdm targetdm = TARGETDM_INITIALIZER;