* Re: [PATCH] jit: Ensure ssize_t is defined.
@ 2024-05-02 19:48 FX Coudert
2024-05-11 15:16 ` FX Coudert
0 siblings, 1 reply; 23+ messages in thread
From: FX Coudert @ 2024-05-02 19:48 UTC (permalink / raw)
To: GCC Patches; +Cc: iain, Eric Gallager
I’d like to ping the patch at https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644134.html
The original proposal by Iain was:
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 235cab053e0..db4f27a48bf 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -21,6 +21,9 @@ along with GCC; see the file COPYING3. If not see
#define LIBGCCJIT_H
#include <stdio.h>
+#if __has_include(<sys/types.h>)
+# include <sys/types.h> /* For ssize_t. */
+#endif
#ifdef __cplusplus
extern "C" {
but it seems we can’t use __has_include. However, other code in GCC treats <sys/types.h> as available on all targets. See unconditional inclusion in gcc/system.h and gcc/tsystem.h. The latter even says:
/* All systems have this header. */
#include <sys/types.h>
So: would an unconditional inclusion be suitable? I’ve tested on linux and darwin with no issues:
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 74e847b2dec..cbe0f70abee 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see
#define LIBGCCJIT_H
#include <stdio.h>
+#include <sys/types.h>
#ifdef __cplusplus
extern "C” {
Thanks,
FX
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] jit: Ensure ssize_t is defined.
2024-05-02 19:48 [PATCH] jit: Ensure ssize_t is defined FX Coudert
@ 2024-05-11 15:16 ` FX Coudert
2024-05-26 15:35 ` FX Coudert
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: FX Coudert @ 2024-05-11 15:16 UTC (permalink / raw)
To: GCC Patches, dmalcolm; +Cc: Iain Sandoe, Eric Gallager
[-- Attachment #1: Type: text/plain, Size: 394 bytes --]
Hi,
On some targets it seems that ssize_t is not defined by any of the headers transitively included by <stdio.h>. This leads to a bootstrap fail when jit is enabled. The attached patch fixes it by include <sys/types.h>. Other headers in GCC treat <sys/types.h> as available on all targets, so we include it unconditionally.
Tested on x86_64-darwin and x86_64-linux. OK to push?
FX
[-- Attachment #2: 0001-jit-Ensure-ssize_t-is-defined.patch --]
[-- Type: application/octet-stream, Size: 983 bytes --]
From 18b6a76a4299e1bb946efc2ce401fc09827e1271 Mon Sep 17 00:00:00 2001
From: Francois-Xavier Coudert <fxcoudert@gcc.gnu.org>
Date: Sat, 11 May 2024 17:08:05 +0200
Subject: [PATCH] jit: Ensure ssize_t is defined
On some targets it seems that ssize_t is not defined by any of the
headers transitively included by <stdio.h>. This leads to a bootstrap
fail when jit is enabled.
Other code in GCC treats <sys/types.h> as available on all targets,
so let's include it unconditionaly.
gcc/jit/ChangeLog:
* libgccjit.h: Include <sys/types.h>
---
gcc/jit/libgccjit.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 74e847b2dec..cbe0f70abee 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see
#define LIBGCCJIT_H
#include <stdio.h>
+#include <sys/types.h>
#ifdef __cplusplus
extern "C" {
--
2.39.3 (Apple Git-146)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-05-11 15:16 ` FX Coudert
@ 2024-05-26 15:35 ` FX Coudert
2024-06-01 16:44 ` FX Coudert
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: FX Coudert @ 2024-05-26 15:35 UTC (permalink / raw)
To: GCC Patches; +Cc: dmalcolm, Iain Sandoe, Eric Gallager
ping
> Le 11 mai 2024 à 17:16, FX Coudert <fxcoudert@gmail.com> a écrit :
>
> Hi,
>
> On some targets it seems that ssize_t is not defined by any of the headers transitively included by <stdio.h>. This leads to a bootstrap fail when jit is enabled. The attached patch fixes it by include <sys/types.h>. Other headers in GCC treat <sys/types.h> as available on all targets, so we include it unconditionally.
>
> Tested on x86_64-darwin and x86_64-linux. OK to push?
>
> FX
>
> <0001-jit-Ensure-ssize_t-is-defined.patch>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-05-11 15:16 ` FX Coudert
2024-05-26 15:35 ` FX Coudert
@ 2024-06-01 16:44 ` FX Coudert
2024-06-11 6:06 ` FX Coudert
2024-06-11 7:20 ` Xi Ruoyao
3 siblings, 0 replies; 23+ messages in thread
From: FX Coudert @ 2024-06-01 16:44 UTC (permalink / raw)
To: GCC Patches; +Cc: dmalcolm, Iain Sandoe, Eric Gallager
ping**2 for this one-liner
> Le 11 mai 2024 à 17:16, FX Coudert <fxcoudert@gmail.com> a écrit :
>
> Hi,
>
> On some targets it seems that ssize_t is not defined by any of the headers transitively included by <stdio.h>. This leads to a bootstrap fail when jit is enabled. The attached patch fixes it by include <sys/types.h>. Other headers in GCC treat <sys/types.h> as available on all targets, so we include it unconditionally.
>
> Tested on x86_64-darwin and x86_64-linux. OK to push?
>
> FX
>
> <0001-jit-Ensure-ssize_t-is-defined.patch>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-05-11 15:16 ` FX Coudert
2024-05-26 15:35 ` FX Coudert
2024-06-01 16:44 ` FX Coudert
@ 2024-06-11 6:06 ` FX Coudert
2024-06-11 7:27 ` Richard Biener
2024-06-11 7:20 ` Xi Ruoyao
3 siblings, 1 reply; 23+ messages in thread
From: FX Coudert @ 2024-06-11 6:06 UTC (permalink / raw)
To: GCC Patches
Cc: dmalcolm, Iain Sandoe, Eric Gallager, rguenther, jakub, josmyers
[-- Attachment #1: Type: text/plain, Size: 612 bytes --]
Hi
I can’t seem to get a review of this one-line patch. Could a global reviewer help?
Thanks,
FX
ping**3
> Le 11 mai 2024 à 17:16, FX Coudert <fxcoudert@gmail.com> a écrit :
>
> Hi,
>
> On some targets it seems that ssize_t is not defined by any of the headers transitively included by <stdio.h>. This leads to a bootstrap fail when jit is enabled. The attached patch fixes it by include <sys/types.h>. Other headers in GCC treat <sys/types.h> as available on all targets, so we include it unconditionally.
>
> Tested on x86_64-darwin and x86_64-linux. OK to push?
>
> FX
>
[-- Attachment #2: 0001-jit-Ensure-ssize_t-is-defined.patch --]
[-- Type: application/octet-stream, Size: 983 bytes --]
From 18b6a76a4299e1bb946efc2ce401fc09827e1271 Mon Sep 17 00:00:00 2001
From: Francois-Xavier Coudert <fxcoudert@gcc.gnu.org>
Date: Sat, 11 May 2024 17:08:05 +0200
Subject: [PATCH] jit: Ensure ssize_t is defined
On some targets it seems that ssize_t is not defined by any of the
headers transitively included by <stdio.h>. This leads to a bootstrap
fail when jit is enabled.
Other code in GCC treats <sys/types.h> as available on all targets,
so let's include it unconditionaly.
gcc/jit/ChangeLog:
* libgccjit.h: Include <sys/types.h>
---
gcc/jit/libgccjit.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 74e847b2dec..cbe0f70abee 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see
#define LIBGCCJIT_H
#include <stdio.h>
+#include <sys/types.h>
#ifdef __cplusplus
extern "C" {
--
2.39.3 (Apple Git-146)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-05-11 15:16 ` FX Coudert
` (2 preceding siblings ...)
2024-06-11 6:06 ` FX Coudert
@ 2024-06-11 7:20 ` Xi Ruoyao
3 siblings, 0 replies; 23+ messages in thread
From: Xi Ruoyao @ 2024-06-11 7:20 UTC (permalink / raw)
To: FX Coudert, GCC Patches, dmalcolm; +Cc: Iain Sandoe, Eric Gallager
On Sat, 2024-05-11 at 17:16 +0200, FX Coudert wrote:
> * libgccjit.h: Include <sys/types.h>
Per the C standard size_t should be provided by stddef.h.
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 6:06 ` FX Coudert
@ 2024-06-11 7:27 ` Richard Biener
2024-06-11 7:44 ` Jakub Jelinek
2024-06-11 8:21 ` FX Coudert
0 siblings, 2 replies; 23+ messages in thread
From: Richard Biener @ 2024-06-11 7:27 UTC (permalink / raw)
To: FX Coudert
Cc: GCC Patches, dmalcolm, Iain Sandoe, Eric Gallager, jakub, josmyers
[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]
On Tue, 11 Jun 2024, FX Coudert wrote:
> Hi
>
> I can’t seem to get a review of this one-line patch. Could a global reviewer help?
While stdio.h can be relied on to exist I do not think you can assume
the same for sys/types.h without "configury", but libgccjit.h is an
installed API. I would assume including stdlib.h gets you ssize_t as
well? In fact the C11 standard doesn't even mention ssize_t so the
API should probably avoid using it and instead use size_t for
/* Given type "T", get its size.
This API entrypoint was added in LIBGCCJIT_ABI_20; you can test for its
presence using
#ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS */
extern ssize_t
gcc_jit_type_get_size (gcc_jit_type *type);
Richard.
> Thanks,
>
> FX
>
>
>
> ping**3
>
>
> > Le 11 mai 2024 à 17:16, FX Coudert <fxcoudert@gmail.com> a écrit :
> >
> > Hi,
> >
> > On some targets it seems that ssize_t is not defined by any of the headers transitively included by <stdio.h>. This leads to a bootstrap fail when jit is enabled. The attached patch fixes it by include <sys/types.h>. Other headers in GCC treat <sys/types.h> as available on all targets, so we include it unconditionally.
> >
> > Tested on x86_64-darwin and x86_64-linux. OK to push?
> >
> > FX
> >
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 7:27 ` Richard Biener
@ 2024-06-11 7:44 ` Jakub Jelinek
2024-06-11 8:03 ` Iain Sandoe
2024-06-27 17:08 ` FX Coudert
2024-06-11 8:21 ` FX Coudert
1 sibling, 2 replies; 23+ messages in thread
From: Jakub Jelinek @ 2024-06-11 7:44 UTC (permalink / raw)
To: Richard Biener
Cc: FX Coudert, GCC Patches, dmalcolm, Iain Sandoe, Eric Gallager, josmyers
On Tue, Jun 11, 2024 at 09:27:37AM +0200, Richard Biener wrote:
> On Tue, 11 Jun 2024, FX Coudert wrote:
>
> > Hi
> >
> > I can’t seem to get a review of this one-line patch. Could a global reviewer help?
>
> While stdio.h can be relied on to exist I do not think you can assume
> the same for sys/types.h without "configury", but libgccjit.h is an
> installed API. I would assume including stdlib.h gets you ssize_t as
> well?
If stdlib.h includes sys/types.h like often on Linux, yes, but not
necessarily. ssize_t is a POSIX type and it might be solely in sys/types.h.
Perhaps libgccjit.h could use
#ifdef __has_include
#if __has_include (<sys/types.h>)
#include <sys/types.h>
#endif
#endif
instead of just #include <sys/types.h>.
When compiled by gcc, one can use hacks like
#define unsigned signed
typedef __SIZE_TYPE__ gcc_jit_ssize_t;
#undef unsigned
but that might not work with other compilers and is perhaps
just too ugly.
> In fact the C11 standard doesn't even mention ssize_t so the
> API should probably avoid using it and instead use size_t for
>
> /* Given type "T", get its size.
> This API entrypoint was added in LIBGCCJIT_ABI_20; you can test for its
> presence using
> #ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS */
> extern ssize_t
> gcc_jit_type_get_size (gcc_jit_type *type);
Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 7:44 ` Jakub Jelinek
@ 2024-06-11 8:03 ` Iain Sandoe
2024-06-11 8:04 ` Richard Biener
2024-06-27 17:08 ` FX Coudert
1 sibling, 1 reply; 23+ messages in thread
From: Iain Sandoe @ 2024-06-11 8:03 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Richard Biener, FX Coudert, GCC Patches, dmalcolm, Eric Gallager,
josmyers
> On 11 Jun 2024, at 08:44, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jun 11, 2024 at 09:27:37AM +0200, Richard Biener wrote:
>> On Tue, 11 Jun 2024, FX Coudert wrote:
>>
>>> Hi
>>>
>>> I can’t seem to get a review of this one-line patch. Could a global reviewer help?
>>
>> While stdio.h can be relied on to exist I do not think you can assume
>> the same for sys/types.h without "configury", but libgccjit.h is an
>> installed API. I would assume including stdlib.h gets you ssize_t as
>> well?
>
> If stdlib.h includes sys/types.h like often on Linux, yes, but not
> necessarily. ssize_t is a POSIX type and it might be solely in sys/types.h.
.. and that is the case for at least some affected Darwin versions (stdlib.h
does not include sys/types.h).
> Perhaps libgccjit.h could use
> #ifdef __has_include
> #if __has_include (<sys/types.h>)
> #include <sys/types.h>
> #endif
> #endif
That seems like a good solution to me.
(my original patch was conditional on __APPLE__ but it seems that the
issue could exist on other platforms too).
Iain
> instead of just #include <sys/types.h>.
> When compiled by gcc, one can use hacks like
> #define unsigned signed
> typedef __SIZE_TYPE__ gcc_jit_ssize_t;
> #undef unsigned
> but that might not work with other compilers and is perhaps
> just too ugly.
>
>> In fact the C11 standard doesn't even mention ssize_t so the
>> API should probably avoid using it and instead use size_t for
>>
>> /* Given type "T", get its size.
>> This API entrypoint was added in LIBGCCJIT_ABI_20; you can test for its
>> presence using
>> #ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS */
>> extern ssize_t
>> gcc_jit_type_get_size (gcc_jit_type *type);
>
> Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 8:03 ` Iain Sandoe
@ 2024-06-11 8:04 ` Richard Biener
2024-06-11 8:06 ` Richard Biener
0 siblings, 1 reply; 23+ messages in thread
From: Richard Biener @ 2024-06-11 8:04 UTC (permalink / raw)
To: Iain Sandoe
Cc: Jakub Jelinek, FX Coudert, GCC Patches, dmalcolm, Eric Gallager,
josmyers
[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]
On Tue, 11 Jun 2024, Iain Sandoe wrote:
>
>
> > On 11 Jun 2024, at 08:44, Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Tue, Jun 11, 2024 at 09:27:37AM +0200, Richard Biener wrote:
> >> On Tue, 11 Jun 2024, FX Coudert wrote:
> >>
> >>> Hi
> >>>
> >>> I can’t seem to get a review of this one-line patch. Could a global reviewer help?
> >>
> >> While stdio.h can be relied on to exist I do not think you can assume
> >> the same for sys/types.h without "configury", but libgccjit.h is an
> >> installed API. I would assume including stdlib.h gets you ssize_t as
> >> well?
> >
> > If stdlib.h includes sys/types.h like often on Linux, yes, but not
> > necessarily. ssize_t is a POSIX type and it might be solely in sys/types.h.
>
> .. and that is the case for at least some affected Darwin versions (stdlib.h
> does not include sys/types.h).
>
> > Perhaps libgccjit.h could use
> > #ifdef __has_include
> > #if __has_include (<sys/types.h>)
> > #include <sys/types.h>
> > #endif
> > #endif
>
> That seems like a good solution to me.
> (my original patch was conditional on __APPLE__ but it seems that the
> issue could exist on other platforms too).
Don't you also need to add
approrpiate #define _POSIX_C_SOURCE or #define _XOPE_SOURCE befor the
include in case somebody builds with -std=c99?
Richard.
> Iain
>
> > instead of just #include <sys/types.h>.
> > When compiled by gcc, one can use hacks like
> > #define unsigned signed
> > typedef __SIZE_TYPE__ gcc_jit_ssize_t;
> > #undef unsigned
> > but that might not work with other compilers and is perhaps
> > just too ugly.
> >
> >> In fact the C11 standard doesn't even mention ssize_t so the
> >> API should probably avoid using it and instead use size_t for
> >>
> >> /* Given type "T", get its size.
> >> This API entrypoint was added in LIBGCCJIT_ABI_20; you can test for its
> >> presence using
> >> #ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS */
> >> extern ssize_t
> >> gcc_jit_type_get_size (gcc_jit_type *type);
> >
> > Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 8:04 ` Richard Biener
@ 2024-06-11 8:06 ` Richard Biener
2024-06-11 8:16 ` Iain Sandoe
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Richard Biener @ 2024-06-11 8:06 UTC (permalink / raw)
To: Iain Sandoe
Cc: Jakub Jelinek, FX Coudert, GCC Patches, dmalcolm, Eric Gallager,
josmyers
[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]
On Tue, 11 Jun 2024, Richard Biener wrote:
> On Tue, 11 Jun 2024, Iain Sandoe wrote:
>
> >
> >
> > > On 11 Jun 2024, at 08:44, Jakub Jelinek <jakub@redhat.com> wrote:
> > >
> > > On Tue, Jun 11, 2024 at 09:27:37AM +0200, Richard Biener wrote:
> > >> On Tue, 11 Jun 2024, FX Coudert wrote:
> > >>
> > >>> Hi
> > >>>
> > >>> I can’t seem to get a review of this one-line patch. Could a global reviewer help?
> > >>
> > >> While stdio.h can be relied on to exist I do not think you can assume
> > >> the same for sys/types.h without "configury", but libgccjit.h is an
> > >> installed API. I would assume including stdlib.h gets you ssize_t as
> > >> well?
> > >
> > > If stdlib.h includes sys/types.h like often on Linux, yes, but not
> > > necessarily. ssize_t is a POSIX type and it might be solely in sys/types.h.
> >
> > .. and that is the case for at least some affected Darwin versions (stdlib.h
> > does not include sys/types.h).
> >
> > > Perhaps libgccjit.h could use
> > > #ifdef __has_include
> > > #if __has_include (<sys/types.h>)
> > > #include <sys/types.h>
> > > #endif
> > > #endif
> >
> > That seems like a good solution to me.
> > (my original patch was conditional on __APPLE__ but it seems that the
> > issue could exist on other platforms too).
>
> Don't you also need to add
>
> approrpiate #define _POSIX_C_SOURCE or #define _XOPE_SOURCE befor the
> include in case somebody builds with -std=c99?
Oh, and the manpage says that <stdio.h> also defines ssize_t which
is a bit odd since we already include that ...
Richard.
> Richard.
>
> > Iain
> >
> > > instead of just #include <sys/types.h>.
> > > When compiled by gcc, one can use hacks like
> > > #define unsigned signed
> > > typedef __SIZE_TYPE__ gcc_jit_ssize_t;
> > > #undef unsigned
> > > but that might not work with other compilers and is perhaps
> > > just too ugly.
> > >
> > >> In fact the C11 standard doesn't even mention ssize_t so the
> > >> API should probably avoid using it and instead use size_t for
> > >>
> > >> /* Given type "T", get its size.
> > >> This API entrypoint was added in LIBGCCJIT_ABI_20; you can test for its
> > >> presence using
> > >> #ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS */
> > >> extern ssize_t
> > >> gcc_jit_type_get_size (gcc_jit_type *type);
> > >
> > > Jakub
> >
> >
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 8:06 ` Richard Biener
@ 2024-06-11 8:16 ` Iain Sandoe
2024-06-11 8:34 ` Andreas Schwab
2024-06-11 8:20 ` Jakub Jelinek
2024-06-11 8:21 ` Andreas Schwab
2 siblings, 1 reply; 23+ messages in thread
From: Iain Sandoe @ 2024-06-11 8:16 UTC (permalink / raw)
To: Richard Biener
Cc: Jakub Jelinek, FX Coudert, GCC Patches, dmalcolm, Eric Gallager,
josmyers
> On 11 Jun 2024, at 09:06, Richard Biener <rguenther@suse.de> wrote:
>
> On Tue, 11 Jun 2024, Richard Biener wrote:
>
>> On Tue, 11 Jun 2024, Iain Sandoe wrote:
>>
>>>
>>>
>>>> On 11 Jun 2024, at 08:44, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>
>>>> On Tue, Jun 11, 2024 at 09:27:37AM +0200, Richard Biener wrote:
>>>>> On Tue, 11 Jun 2024, FX Coudert wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> I can’t seem to get a review of this one-line patch. Could a global reviewer help?
>>>>>
>>>>> While stdio.h can be relied on to exist I do not think you can assume
>>>>> the same for sys/types.h without "configury", but libgccjit.h is an
>>>>> installed API. I would assume including stdlib.h gets you ssize_t as
>>>>> well?
>>>>
>>>> If stdlib.h includes sys/types.h like often on Linux, yes, but not
>>>> necessarily. ssize_t is a POSIX type and it might be solely in sys/types.h.
>>>
>>> .. and that is the case for at least some affected Darwin versions (stdlib.h
>>> does not include sys/types.h).
>>>
>>>> Perhaps libgccjit.h could use
>>>> #ifdef __has_include
>>>> #if __has_include (<sys/types.h>)
>>>> #include <sys/types.h>
>>>> #endif
>>>> #endif
>>>
>>> That seems like a good solution to me.
>>> (my original patch was conditional on __APPLE__ but it seems that the
>>> issue could exist on other platforms too).
>>
>> Don't you also need to add
>>
>> approrpiate #define _POSIX_C_SOURCE or #define _XOPE_SOURCE befor the
>> include in case somebody builds with -std=c99?
well, afaict, all the code is c++ and we are building with a std >= 11, so that
presumes c99 support. I can confirm that, at least on Darwin, we do not need
to define either _POSIX_C_SOURCE, or _XOPEN_SOURCE to get the
correct behaviour.
> Oh, and the manpage says that <stdio.h> also defines ssize_t which
> is a bit odd since we already include that ...
that seems to be the case for darwin >= 11 but not before.
Iain
>
> Richard.
>
>> Richard.
>>
>>> Iain
>>>
>>>> instead of just #include <sys/types.h>.
>>>> When compiled by gcc, one can use hacks like
>>>> #define unsigned signed
>>>> typedef __SIZE_TYPE__ gcc_jit_ssize_t;
>>>> #undef unsigned
>>>> but that might not work with other compilers and is perhaps
>>>> just too ugly.
>>>>
>>>>> In fact the C11 standard doesn't even mention ssize_t so the
>>>>> API should probably avoid using it and instead use size_t for
>>>>>
>>>>> /* Given type "T", get its size.
>>>>> This API entrypoint was added in LIBGCCJIT_ABI_20; you can test for its
>>>>> presence using
>>>>> #ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS */
>>>>> extern ssize_t
>>>>> gcc_jit_type_get_size (gcc_jit_type *type);
>>>>
>>>> Jakub
>>>
>>>
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 8:06 ` Richard Biener
2024-06-11 8:16 ` Iain Sandoe
@ 2024-06-11 8:20 ` Jakub Jelinek
2024-06-11 8:21 ` Andreas Schwab
2 siblings, 0 replies; 23+ messages in thread
From: Jakub Jelinek @ 2024-06-11 8:20 UTC (permalink / raw)
To: Richard Biener
Cc: Iain Sandoe, FX Coudert, GCC Patches, dmalcolm, Eric Gallager, josmyers
On Tue, Jun 11, 2024 at 10:06:49AM +0200, Richard Biener wrote:
> > approrpiate #define _POSIX_C_SOURCE or #define _XOPE_SOURCE befor the
> > include in case somebody builds with -std=c99?
>
> Oh, and the manpage says that <stdio.h> also defines ssize_t which
> is a bit odd since we already include that ...
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdio.h.html
shows that indeed POSIX 2018 stdio.h should provide ssize_t, but
e.g. POSIX 2004 stdio.h doesn't have to:
https://pubs.opengroup.org/onlinepubs/007904875/basedefs/stdio.h.html
Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 8:06 ` Richard Biener
2024-06-11 8:16 ` Iain Sandoe
2024-06-11 8:20 ` Jakub Jelinek
@ 2024-06-11 8:21 ` Andreas Schwab
2 siblings, 0 replies; 23+ messages in thread
From: Andreas Schwab @ 2024-06-11 8:21 UTC (permalink / raw)
To: Richard Biener
Cc: Iain Sandoe, Jakub Jelinek, FX Coudert, GCC Patches, dmalcolm,
Eric Gallager, josmyers
On Jun 11 2024, Richard Biener wrote:
>> Don't you also need to add
>>
>> approrpiate #define _POSIX_C_SOURCE or #define _XOPE_SOURCE befor the
>> include in case somebody builds with -std=c99?
Such feature macros can only be defined before the very first include of
a system header.
> Oh, and the manpage says that <stdio.h> also defines ssize_t which
> is a bit odd since we already include that ...
Only since POSIX.1-2008.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 7:27 ` Richard Biener
2024-06-11 7:44 ` Jakub Jelinek
@ 2024-06-11 8:21 ` FX Coudert
1 sibling, 0 replies; 23+ messages in thread
From: FX Coudert @ 2024-06-11 8:21 UTC (permalink / raw)
To: Richard Biener
Cc: GCC Patches, dmalcolm, Iain Sandoe, Eric Gallager, jakub, josmyers
> While stdio.h can be relied on to exist I do not think you can assume
> the same for sys/types.h without "configury", but libgccjit.h is an
> installed API.
sys/types.h is already included unconditionally in gcc/system.h and gcc/tsystem.h. The later says:
/* All systems have this header. */
#include <sys/types.h>
FX
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 8:16 ` Iain Sandoe
@ 2024-06-11 8:34 ` Andreas Schwab
0 siblings, 0 replies; 23+ messages in thread
From: Andreas Schwab @ 2024-06-11 8:34 UTC (permalink / raw)
To: Iain Sandoe
Cc: Richard Biener, Jakub Jelinek, FX Coudert, GCC Patches, dmalcolm,
Eric Gallager, josmyers
On Jun 11 2024, Iain Sandoe wrote:
> well, afaict, all the code is c++ and we are building with a std >= 11, so that
> presumes c99 support.
The C standard does not define ssize_t at all, it is only part of POSIX.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-11 7:44 ` Jakub Jelinek
2024-06-11 8:03 ` Iain Sandoe
@ 2024-06-27 17:08 ` FX Coudert
2024-06-28 6:17 ` Richard Biener
1 sibling, 1 reply; 23+ messages in thread
From: FX Coudert @ 2024-06-27 17:08 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Richard Biener, GCC Patches, dmalcolm, Iain Sandoe,
Eric Gallager, josmyers
[-- Attachment #1: Type: text/plain, Size: 482 bytes --]
Among the review comments from the last round, Jakub suggested:
> Perhaps libgccjit.h could use
> #ifdef __has_include
> #if __has_include (<sys/types.h>)
> #include <sys/types.h>
> #endif
> #endif
> instead of just #include <sys/types.h>.
I’m not sure it’s necessary since other headers treat <sys/types.h> as always available, but I suppose it can’t hurt. So here is a revised patch as suggested.
Bootstrapped on x86_64-apple-darwin23. OK to push?
FX
[-- Attachment #2: 0001-jit-Ensure-ssize_t-is-defined.patch --]
[-- Type: application/octet-stream, Size: 958 bytes --]
From b06cdbb782514a37bb196770a8599f35bcbfc07e Mon Sep 17 00:00:00 2001
From: Francois-Xavier Coudert <fxcoudert@gcc.gnu.org>
Date: Sat, 11 May 2024 17:08:05 +0200
Subject: [PATCH] jit: Ensure ssize_t is defined
On some targets it seems that ssize_t is not defined by any of the
headers transitively included by <stdio.h>. This leads to a bootstrap
fail when jit is enabled.
gcc/jit/ChangeLog:
* libgccjit.h: Include <sys/types.h>
---
gcc/jit/libgccjit.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 74e847b2dec..91184f973fd 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -21,6 +21,11 @@ along with GCC; see the file COPYING3. If not see
#define LIBGCCJIT_H
#include <stdio.h>
+#ifdef __has_include
+#if __has_include (<sys/types.h>)
+#include <sys/types.h>
+#endif
+#endif
#ifdef __cplusplus
extern "C" {
--
2.39.3 (Apple Git-146)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-27 17:08 ` FX Coudert
@ 2024-06-28 6:17 ` Richard Biener
2024-06-28 7:15 ` FX Coudert
0 siblings, 1 reply; 23+ messages in thread
From: Richard Biener @ 2024-06-28 6:17 UTC (permalink / raw)
To: FX Coudert
Cc: Jakub Jelinek, GCC Patches, dmalcolm, Iain Sandoe, Eric Gallager,
josmyers
[-- Attachment #1: Type: text/plain, Size: 868 bytes --]
On Thu, 27 Jun 2024, FX Coudert wrote:
> Among the review comments from the last round, Jakub suggested:
>
> > Perhaps libgccjit.h could use
> > #ifdef __has_include
> > #if __has_include (<sys/types.h>)
> > #include <sys/types.h>
> > #endif
> > #endif
> > instead of just #include <sys/types.h>.
>
> I’m not sure it’s necessary since other headers treat <sys/types.h> as always available, but I suppose it can’t hurt. So here is a revised patch as suggested.
But isn't the bigger issue that sys/types.h isn't guaranteed to contain
a declaration of ssize_t? And that when sys/types.h isn't available
we don't get ssize_t from it either?
That said, maybe we're fine with this but then I walk back and say
just unconditionally include sys/types.h ...
> Bootstrapped on x86_64-apple-darwin23. OK to push?
Should be Davids say as he added this API.
Richard.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-06-28 6:17 ` Richard Biener
@ 2024-06-28 7:15 ` FX Coudert
0 siblings, 0 replies; 23+ messages in thread
From: FX Coudert @ 2024-06-28 7:15 UTC (permalink / raw)
To: Richard Biener
Cc: Jakub Jelinek, GCC Patches, dmalcolm, Iain Sandoe, Eric Gallager,
josmyers
> But isn't the bigger issue that sys/types.h isn't guaranteed to contain
> a declaration of ssize_t? And that when sys/types.h isn't available
> we don't get ssize_t from it either?
Some targets seem to get it indirectly from stdio.h
As far as I know, darwin is the only platform broken currently, and the patch is definitely an improvement there.
> That said, maybe we're fine with this but then I walk back and say
> just unconditionally include sys/types.h ...
It is included unconditionally in other headers, yes.
> Should be Davids say as he added this API.
Agreed, but a version of the patch was original proposed in January by Iain Sandoe, and I have proposed and pinged another version since May 2nd, and got no response from him. It is breaking build on a
FX
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-01-28 23:13 ` Iain Sandoe
@ 2024-01-29 11:26 ` Iain Sandoe
0 siblings, 0 replies; 23+ messages in thread
From: Iain Sandoe @ 2024-01-29 11:26 UTC (permalink / raw)
To: dmalcolm; +Cc: Eric Gallager, GCC Patches
Hi David,
I guess the solution here depends on the scope over which we expect
the header to be used.
> On 28 Jan 2024, at 23:13, Iain Sandoe <iain@sandoe.co.uk> wrote:
>> On 28 Jan 2024, at 21:25, Eric Gallager <egall@gwmail.gwu.edu> wrote:
>> On Sun, Jan 28, 2024 at 6:45 AM Iain Sandoe <iains.gcc@gmail.com> wrote:
>>>
>>> Tested on i686, x86_64 Darwin, x86_64 Linux,
>>> OK for trunk?
>>>
>>> --- 8< ---
>>>
>>> On some targets it seems that ssize_t is not defined by any of the
>>> headers transitively included by <stdio.h>. This leads to a bootstrap
>>> fail when jit is enabled.
>>>
>>> The fix proposed here is to include sys/types.h when it is available
>>> since that is where Posix specifies that ssize_t is defined.
>>>
>>> gcc/jit/ChangeLog:
>>>
>>> * libgccjit.h: Conditionally include <sys/types.h> where it is
>>> available to ensure declaration of ssize_t.
>>>
>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>> ---
>>> gcc/jit/libgccjit.h | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
>>> index 235cab053e0..db4f27a48bf 100644
>>> --- a/gcc/jit/libgccjit.h
>>> +++ b/gcc/jit/libgccjit.h
>>> @@ -21,6 +21,9 @@ along with GCC; see the file COPYING3. If not see
>>> #define LIBGCCJIT_H
>>>
>>> #include <stdio.h>
>>> +#if __has_include(<sys/types.h>)
>>
>> Is __has_include() something that we can use unconditionally?
>
> Hmm.. maybe we cannot, it seems it was introduced in gcc-4.9 and we only ask
> for 4.8, IIRC.
>
> I guess HAVE_SYS_TYPES_H might be an alternative (I’ll have to retest)
Answering my own question; no that is not going to work either since the header is
installed and config.h is not.
I guess the question is “is this header ever [meaningfully] consumed by a compiler
other than the current GCC that it supports”?
e.g. if we expected we could build libgccjit with clang in a “—disable-bootstrap”
configuration and expect that to work?
The fallback is
#ifdef __APPLE__
# include <sys/types.h> /* For ssize_t. */
#endif
(which I will test on a number of platform versions).
since this breaks bootstrap at stage 2 on affected platform versions, so we need some
fix.
thanks
Iain
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-01-28 21:25 ` Eric Gallager
@ 2024-01-28 23:13 ` Iain Sandoe
2024-01-29 11:26 ` Iain Sandoe
0 siblings, 1 reply; 23+ messages in thread
From: Iain Sandoe @ 2024-01-28 23:13 UTC (permalink / raw)
To: Eric Gallager, GCC Patches; +Cc: dmalcolm
> On 28 Jan 2024, at 21:25, Eric Gallager <egall@gwmail.gwu.edu> wrote:
>
> On Sun, Jan 28, 2024 at 6:45 AM Iain Sandoe <iains.gcc@gmail.com> wrote:
>>
>> Tested on i686, x86_64 Darwin, x86_64 Linux,
>> OK for trunk?
>>
>> --- 8< ---
>>
>> On some targets it seems that ssize_t is not defined by any of the
>> headers transitively included by <stdio.h>. This leads to a bootstrap
>> fail when jit is enabled.
>>
>> The fix proposed here is to include sys/types.h when it is available
>> since that is where Posix specifies that ssize_t is defined.
>>
>> gcc/jit/ChangeLog:
>>
>> * libgccjit.h: Conditionally include <sys/types.h> where it is
>> available to ensure declaration of ssize_t.
>>
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> ---
>> gcc/jit/libgccjit.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
>> index 235cab053e0..db4f27a48bf 100644
>> --- a/gcc/jit/libgccjit.h
>> +++ b/gcc/jit/libgccjit.h
>> @@ -21,6 +21,9 @@ along with GCC; see the file COPYING3. If not see
>> #define LIBGCCJIT_H
>>
>> #include <stdio.h>
>> +#if __has_include(<sys/types.h>)
>
> Is __has_include() something that we can use unconditionally?
Hmm.. maybe we cannot, it seems it was introduced in gcc-4.9 and we only ask
for 4.8, IIRC.
I guess HAVE_SYS_TYPES_H might be an alternative (I’ll have to retest)
Iain
>
>> +# include <sys/types.h> /* For ssize_t. */
>> +#endif
>>
>> #ifdef __cplusplus
>> extern "C" {
>> --
>> 2.39.2 (Apple Git-143)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] jit: Ensure ssize_t is defined.
2024-01-28 11:44 Iain Sandoe
@ 2024-01-28 21:25 ` Eric Gallager
2024-01-28 23:13 ` Iain Sandoe
0 siblings, 1 reply; 23+ messages in thread
From: Eric Gallager @ 2024-01-28 21:25 UTC (permalink / raw)
To: iain; +Cc: gcc-patches, dmalcolm
On Sun, Jan 28, 2024 at 6:45 AM Iain Sandoe <iains.gcc@gmail.com> wrote:
>
> Tested on i686, x86_64 Darwin, x86_64 Linux,
> OK for trunk?
>
> --- 8< ---
>
> On some targets it seems that ssize_t is not defined by any of the
> headers transitively included by <stdio.h>. This leads to a bootstrap
> fail when jit is enabled.
>
> The fix proposed here is to include sys/types.h when it is available
> since that is where Posix specifies that ssize_t is defined.
>
> gcc/jit/ChangeLog:
>
> * libgccjit.h: Conditionally include <sys/types.h> where it is
> available to ensure declaration of ssize_t.
>
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> ---
> gcc/jit/libgccjit.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 235cab053e0..db4f27a48bf 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -21,6 +21,9 @@ along with GCC; see the file COPYING3. If not see
> #define LIBGCCJIT_H
>
> #include <stdio.h>
> +#if __has_include(<sys/types.h>)
Is __has_include() something that we can use unconditionally?
> +# include <sys/types.h> /* For ssize_t. */
> +#endif
>
> #ifdef __cplusplus
> extern "C" {
> --
> 2.39.2 (Apple Git-143)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] jit: Ensure ssize_t is defined.
@ 2024-01-28 11:44 Iain Sandoe
2024-01-28 21:25 ` Eric Gallager
0 siblings, 1 reply; 23+ messages in thread
From: Iain Sandoe @ 2024-01-28 11:44 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm
Tested on i686, x86_64 Darwin, x86_64 Linux,
OK for trunk?
--- 8< ---
On some targets it seems that ssize_t is not defined by any of the
headers transitively included by <stdio.h>. This leads to a bootstrap
fail when jit is enabled.
The fix proposed here is to include sys/types.h when it is available
since that is where Posix specifies that ssize_t is defined.
gcc/jit/ChangeLog:
* libgccjit.h: Conditionally include <sys/types.h> where it is
available to ensure declaration of ssize_t.
Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
---
gcc/jit/libgccjit.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 235cab053e0..db4f27a48bf 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -21,6 +21,9 @@ along with GCC; see the file COPYING3. If not see
#define LIBGCCJIT_H
#include <stdio.h>
+#if __has_include(<sys/types.h>)
+# include <sys/types.h> /* For ssize_t. */
+#endif
#ifdef __cplusplus
extern "C" {
--
2.39.2 (Apple Git-143)
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2024-06-28 7:16 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-02 19:48 [PATCH] jit: Ensure ssize_t is defined FX Coudert
2024-05-11 15:16 ` FX Coudert
2024-05-26 15:35 ` FX Coudert
2024-06-01 16:44 ` FX Coudert
2024-06-11 6:06 ` FX Coudert
2024-06-11 7:27 ` Richard Biener
2024-06-11 7:44 ` Jakub Jelinek
2024-06-11 8:03 ` Iain Sandoe
2024-06-11 8:04 ` Richard Biener
2024-06-11 8:06 ` Richard Biener
2024-06-11 8:16 ` Iain Sandoe
2024-06-11 8:34 ` Andreas Schwab
2024-06-11 8:20 ` Jakub Jelinek
2024-06-11 8:21 ` Andreas Schwab
2024-06-27 17:08 ` FX Coudert
2024-06-28 6:17 ` Richard Biener
2024-06-28 7:15 ` FX Coudert
2024-06-11 8:21 ` FX Coudert
2024-06-11 7:20 ` Xi Ruoyao
-- strict thread matches above, loose matches on Subject: below --
2024-01-28 11:44 Iain Sandoe
2024-01-28 21:25 ` Eric Gallager
2024-01-28 23:13 ` Iain Sandoe
2024-01-29 11:26 ` Iain Sandoe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).