Re: [PATCH, Fortran] Bind(c): CFI_signed_char is not a Fortran character type
Hi Sandra, The part of the patch to add tests for this goes on top of my base TS29113 testsuite patch, which hasn't been reviewed or committed yet. It is my understanding that it is not gcc policy to add xfailed test cases for things that do not yet work. Rather, xfail is for tests that later turn out not to work, especially on certain architectures. I have added Toon in CC, maybe he can explain a bit more on that. Regards Thomas
Re: *Ping**2 [Patch] Fortran: Fix bind(C) character length checks
Good to go Tobias.
Jerry
On 7/14/21 5:50 AM, Burnus, Tobias wrote:
Ping**2
On Juli 8, 2021 I wrote:
*Ping*
I intent to incorporate Sandra's suggestions, except for the beginning of line
spacing - that's needed to avoid exceeding the 80 character line limit. I did
not include an updated patch as just pinging is easier on a mobile during
vacation :-)
Thanks,
Tobias
Loosemore, Sandra wrote:
On 7/1/21 11:08 AM, Tobias Burnus wrote:
Hi all,
this patch came up when discussing Sandra's TS29113 patch internally.
There is presumably also some overlap with José's patches.
This patch tries to rectify the BIND(C) CHARACTER handling on the
diagnostic side, only. That is: what to accept and what
to reject for which Fortran standard.
The rules are:
* [F2003-F2018] Interoperable is character(len=1)
→ F2018, 18.3.1 Interoperability of intrinsic types
(General, unchanged)
* Fortran 2008: In some cases, const-length chars are
permitted as well:
→ F2018, 18.3.4 Interoperability of scalar variables
→ F2018, 18.3.5 Interoperability of array variables
→ F2018, 18.3.6 Interoperability of procedures and procedure interfaces
[= F2008, 15.3.{4,5,6}
For global vars with bind(C), 18.3.4 + 18.3.5 applies directly (TODO:
Add support, not in this patch)
For passed-by ref dummy arguments, 18.3.4 + 18.3.5 are referenced in
- F2008: R1229 proc-language-binding-spec is language-binding-spec
C1255 (R1229)
- F2018, F2018, C1554
While it is not very clearly spelt out, I regard 'char parm[4]'
interoperable with 'character(len=4) :: a', 'character(len=2) :: b(2)'
and 'character(len=1) :: c(4)' for both global variables and for
dummy arguments.
* Fortran 2018/TS29113: Uses additionally CFI array descriptor
- allocatable, pointer: must be len=:
- nonallocatable/nonpointer: len=* → implies array descriptor also
for assumed-size/explicit-size/scalar arguments.
- All which all passed by an array descriptor already without further
restrictions: assumed-shape, assumed-rank, i.e. len= seems
to be also fine
→ 18.3.6 under item (5) bullet point 2 and 3 plus (6).
I hope I got the conditions right. I also fixed an issue with
character(len=5) :: str – the code in trans-expr.c did crash for
scalars (decl.c did not check any constraints for arrays).
I believe the condition is wrong and for len= no descriptor
is used.
Any comments, remarks?
I gave this patch a try on my TS 29113 last night. Changing the error
messages kind of screwed up my list of FAILs, but I did see that it also
caught some invalid character arguments in
interoperability/typecodes-scalar.f90 and
interoperability/typecodes-scalar-ext.f90 (which are already broken by 2
other major gfortran bugs I still need to file PRs for). :-S
I haven't tried to review the patch WRT correctness with the
requirements of the standard yet, but I have a few nits about error
messages
+ /* F2018, 18.3.6 (6). */
+ if (!sym->ts.deferred)
+ {
+ gfc_error ("Allocatable and pointer character dummy "
+ "argument %qs at %L must have deferred length "
+ "as procedure %qs is BIND(C)", sym->name,
+ &sym->declared_at, sym->ns->proc_name->name);
+ retval = false;
+ }
This is the error the two aforementioned test cases started giving, but
message is confusing and doesn't read well (it was a pointer dummy, not
"allocatable and pointer"). Maybe just s/and/or/, or customize the
message depending on which one it is?
+ gfc_error ("Character dummy argument %qs at %L must be "
+ "of constant length or assumed length, "
+ "unless it has assumed-shape or assumed-rank, "
+ "as procedure %qs has the BIND(C) attribute",
+ sym->name, &sym->declared_at,
+ sym->ns->proc_name->name);
I don't think either "assumed-shape" or "assumed-rank" should be
hyphenated in this context unless that exact hyphenation is a term of
art in the Fortran standard or other technical documentation. In normal
English, adjective phrases are usually only hyphenated when they appear
immediately before the noun they modify; "assumed-shape array", but "an
array with assumed shape".
+ else if (!gfc_notify_std (GFC_STD_F2018,
+ "Character dummy argument %qs at %L"
+ " with nonconstant length as "
+ "procedure %qs is BIND(C)",
+ sym->name, &sym->declared_at,
+ sym->ns->proc_name->name))
+ retval = false;
+ }
Elsewhere the convention seems to be to format strings split across
mu
Pushing XFAILed test cases (was: [PATCH, Fortran] Bind(c): CFI_signed_char is not a Fortran character type)
[Also including for guidance.]
Hi!
(I'm not involved in or familiar with Sandra's Fortran TS29113 work, just
commenting generally here.)
On 2021-07-16T09:52:28+0200, Thomas Koenig via Gcc-patches
wrote:
> It is my understanding that it is not gcc policy to add xfailed test
> cases for things that do not yet work. Rather, xfail is for tests that
> later turn out not to work, especially on certain architectures.
That's not current practice, as far as I can tell. I'm certainly
"guilty" of pushing lots of XFAILed test cases (or, most often,
individual XFAILed DejaGnu directives), and I see a good number of others
GCC folks do that, too. Ideally with but casually also without
corresponding GCC PRs filed. If without, then of course should have
suitable commentary inside the test case file. Time span of addressing
the XFAILs ranging between days and years.
In my opinion, if a test case has been written and analyzed, why
shouldn't you push it, even if (parts of) it don't quite work yet? (If
someone -- at another time, possibly -- then implements the missing
functionality/fixes the bugs, the XFAILs turn into XPASSes, thus serving
to demonstrate the effect of code changes.
Otherwise -- and I've run into that just yesterday... -- effort spent on
such test cases simply gets lost "in the noise of the mailing list
archives", until re-discovered, or -- in my case -- re-implemented and
then re-discovered by chance.
We nowadays even have a way to mark up ICEing test cases ('dg-ice'),
which has been used to push test cases that ICE for '{ target *-*-* }'.
Of course, we shall assume a certain level of quality in the XFAILed test
cases: I'm certainly not suggesting we put any random junk into the
testsuite, coarsely XFAILed. (I have not reviewed Sandra's test cases to
that effect, but knowing here, I'd be surprised if that were the problem
here.)
Not trying to overrule you, just sharing my opinion -- now happy to hear
others. :-)
Grüße
Thomas
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
Re: Pushing XFAILed test cases
On 7/16/21 9:32 AM, Thomas Schwinge wrote:
[Also including for guidance.]
Hi!
(I'm not involved in or familiar with Sandra's Fortran TS29113 work, just
commenting generally here.)
On 2021-07-16T09:52:28+0200, Thomas Koenig via Gcc-patches
wrote:
It is my understanding that it is not gcc policy to add xfailed test
cases for things that do not yet work. Rather, xfail is for tests that
later turn out not to work, especially on certain architectures.
That's not current practice, as far as I can tell. I'm certainly
"guilty" of pushing lots of XFAILed test cases (or, most often,
individual XFAILed DejaGnu directives), and I see a good number of others
GCC folks do that, too. Ideally with but casually also without
corresponding GCC PRs filed. If without, then of course should have
suitable commentary inside the test case file. Time span of addressing
the XFAILs ranging between days and years.
In my opinion, if a test case has been written and analyzed, why
shouldn't you push it, even if (parts of) it don't quite work yet? (If
someone -- at another time, possibly -- then implements the missing
functionality/fixes the bugs, the XFAILs turn into XPASSes, thus serving
to demonstrate the effect of code changes.
Otherwise -- and I've run into that just yesterday... -- effort spent on
such test cases simply gets lost "in the noise of the mailing list
archives", until re-discovered, or -- in my case -- re-implemented and
then re-discovered by chance.
We nowadays even have a way to mark up ICEing test cases ('dg-ice'),
which has been used to push test cases that ICE for '{ target *-*-* }'.
Of course, we shall assume a certain level of quality in the XFAILed test
cases: I'm certainly not suggesting we put any random junk into the
testsuite, coarsely XFAILed. (I have not reviewed Sandra's test cases to
that effect, but knowing here, I'd be surprised if that were the problem
here.)
Not trying to overrule you, just sharing my opinion -- now happy to hear
others. :-)
I've also been xfailing individual directives in new tests, with
or without PRs tracking the corresponding limitations (not so much
outright bugs as future enhancements). The practice has been
discussed in the past and (IIRC) there was general agreement with
it. Marek even formalized some of it for the C++ front end by
adding support for one or more dg- directives (I think dg-ice was
one of them). The discussion I recall is here:
https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550913.html
Martin
Re: Pushing XFAILed test cases
On 7/16/21 9:32 AM, Thomas Schwinge wrote: [much snipped] Of course, we shall assume a certain level of quality in the XFAILed test cases: I'm certainly not suggesting we put any random junk into the testsuite, coarsely XFAILed. (I have not reviewed Sandra's test cases to that effect, but knowing here, I'd be surprised if that were the problem here.) FWIW, Tobias already did an extensive review of an early version of the testsuite patches in question and pointed out several cases where failures were due to my misunderstanding of the language standard or general confusion about what the expected behavior was supposed to be when gfortran wasn't implementing it or was tripping over other bugs. :-S I hope I incorporated all his suggestions and rewrote the previously-bogus tests to be more useful for the version I posted for review on the Fortran list, but shouldn't the normal patch review process be adequate to take care of any additional concerns about quality? My previous understanding of the development process and testsuite conventions is that adding tests that FAIL is bad, but XFAILing them with reference to a PR is OK, and certainly much better than simply not having test coverage of those things at all. Especially in the case of something like the TS29113 testsuite where the explicit goal is to track standards compliance and/or the completeness of the existing implementation. :-S So it seems to me rather surprising to take the position that we should not be committing any new test cases that need to be XFAILed. :-S -Sandra
[PATCH, Fortran] [PR libfortran/101317] Bind(c): Improve error checking in CFI_* functions
This patch is for PR101317, one of the bugs uncovered by the TS29113
testsuite. Here I'd observed that CFI_establish, etc was not diagnosing
some invalid-argument situations documented in the standard, although it
was properly catching others. After fixing those I discovered a couple
small mistakes in the test cases and fixed those too.
The testsuite fixes can either be committed with this patch or rolled
into the TS29113 testsuite, depending on the order in which things are
approved/committed.
OK?
-Sandra
commit 6cecb3e3625072c7846434df9dcd8db5e6f66432
Author: Sandra Loosemore
Date: Thu Jul 15 08:48:45 2021 -0700
[PR libfortran/101317] Bind(c): Improve error checking in CFI_* functions
This patch adds additional run-time checking for invalid arguments to
CFI_allocate, CFI_establish, CFI_select_part, and CFI_setpointer.
It also includes some minor fixes for signed/unsigned confusion in the
TS29113 testsuite.
2021-07-16 Sandra Loosemore
PR libfortran/101317
libgfortran/
* runtime/ISO_Fortran_binding.c (CFI_allocate): Check elem_len
for validity when it's used.
(CFI_establish): Likewise. Also check type argument and extents.
(CFI_select_part): Check elem_len.
(CFI_setpointer): Check that source is not an unallocated
allocatable array or an assumed-size array. Minor formatting
cleanup.
gcc/testsuite/
* gfortran.dg/ts29113/library/establish-errors-c.c (ctest):
Correct unsigned argument to CFI_establish.
* gfortran.dg/ts29113/library/setpointer-errors-c.c (ctest):
Bypass CFI_establish to create an assumed-size array.
diff --git a/gcc/testsuite/gfortran.dg/ts29113/library/establish-errors-c.c b/gcc/testsuite/gfortran.dg/ts29113/library/establish-errors-c.c
index b55362a..ae02b46 100644
--- a/gcc/testsuite/gfortran.dg/ts29113/library/establish-errors-c.c
+++ b/gcc/testsuite/gfortran.dg/ts29113/library/establish-errors-c.c
@@ -57,7 +57,7 @@ ctest (void)
character type, elem_len shall be greater than zero and equal to
the storage size in bytes of an element of the object. */
status = CFI_establish (a, (void *)buf, CFI_attribute_other,
- CFI_type_struct, -5, 2, extents);
+ CFI_type_struct, 0, 2, extents);
if (status == CFI_SUCCESS)
{
fprintf (stderr,
diff --git a/gcc/testsuite/gfortran.dg/ts29113/library/setpointer-errors-c.c b/gcc/testsuite/gfortran.dg/ts29113/library/setpointer-errors-c.c
index eec96e6..670d360 100644
--- a/gcc/testsuite/gfortran.dg/ts29113/library/setpointer-errors-c.c
+++ b/gcc/testsuite/gfortran.dg/ts29113/library/setpointer-errors-c.c
@@ -8,7 +8,6 @@
static int a[10][5][3];
static CFI_index_t extents[] = {3, 5, 10};
-static CFI_index_t badextents[] = {3, 5, -1};
/* External entry point. */
extern void ctest (void);
@@ -69,9 +68,12 @@ ctest (void)
bad ++;
}
+ /* CFI_establish rejects negative extents, so we can't use it to make
+ an assumed-size array, so hack the descriptor by hand. Yuck. */
check_CFI_status ("CFI_establish",
CFI_establish (source, (void *)a, CFI_attribute_other,
- CFI_type_int, 0, 3, badextents));
+ CFI_type_int, 0, 3, extents));
+ source->dim[2].extent = -1;
status = CFI_setpointer (result, source, NULL);
if (status == CFI_SUCCESS)
{
diff --git a/libgfortran/runtime/ISO_Fortran_binding.c b/libgfortran/runtime/ISO_Fortran_binding.c
index 79bb377..38e1b6e 100644
--- a/libgfortran/runtime/ISO_Fortran_binding.c
+++ b/libgfortran/runtime/ISO_Fortran_binding.c
@@ -232,7 +232,16 @@ CFI_allocate (CFI_cdesc_t *dv, const CFI_index_t lower_bounds[],
/* If the type is a Fortran character type, the descriptor's element
length is replaced by the elem_len argument. */
if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char)
-dv->elem_len = elem_len;
+{
+ if (unlikely (compile_options.bounds_check) && elem_len == 0)
+ {
+ fprintf ("CFI_allocate: The supplied elem_len must be "
+ "greater than zero (elem_len = %d).\n",
+ (int) elem_len);
+ return CFI_INVALID_ELEM_LEN;
+ }
+ dv->elem_len = elem_len;
+}
/* Dimension information and calculating the array length. */
size_t arr_len = 1;
@@ -342,11 +351,28 @@ int CFI_establish (CFI_cdesc_t *dv, void *base_addr, CFI_attribute_t attribute,
if (type == CFI_type_char || type == CFI_type_ucs4_char
|| type == CFI_type_struct || type == CFI_type_other)
-dv->elem_len = elem_len;
+{
+ /* Note that elem_len has type size_t, which is unsigned. */
+ if (unlikely (compile_options.bounds_check) && elem_len == 0)
+ {
+ fprintf ("CFI_establish: The supplied elem_len must be "
+ "greater than zero (elem_len = %d).\n",
+ (int) elem_len);
+ return CFI_INVALID_ELEM_LEN;
+ }
+ dv->elem_len = elem_len;
+}
else if (type == CFI_type_cptr)
dv->elem_len = sizeof (void *);
else if (type == CFI_type_cfunptr)
