Re: [PATCH] jit: Ensure ssize_t is defined.

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

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

[PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

[-- 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

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>

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

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>

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

[-- 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Xi Ruoyao]

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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Richard Biener]

[-- 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Jakub Jelinek]

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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Iain Sandoe]

> 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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Richard Biener]

[-- 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Richard Biener]

[-- 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Iain Sandoe]

> 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Jakub Jelinek]

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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Andreas Schwab]

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."

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

> 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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Andreas Schwab]

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."

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

[-- 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Richard Biener]

[-- 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.

Re: [PATCH] jit: Ensure ssize_t is defined.

[FX Coudert]

> 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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Iain Sandoe]

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

Re: [PATCH] jit: Ensure ssize_t is defined.

[Iain Sandoe]

> 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)

Re: [PATCH] jit: Ensure ssize_t is defined.

[Eric Gallager]

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)
>

[PATCH] jit: Ensure ssize_t is defined.

[Iain Sandoe]

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)