| Message ID | 13168f92-8863-cb63-9470-a6055d5da5f6@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Fortran] Bind(c): CFI_signed_char is not a Fortran character type | expand |
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
[Also including <gcc@gcc.gnu.org> 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 <gcc-patches@gcc.gnu.org> 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
On 7/16/21 9:32 AM, Thomas Schwinge wrote: > [Also including <gcc@gcc.gnu.org> 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 <gcc-patches@gcc.gnu.org> 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
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
On 16.07.21 20:22, Sandra Loosemore wrote: > 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 It is what I was told in no uncertain terms some years ago, which is where my current state of knowledge comes from. So, I have added the gcc mailing list to this discussion, with a general question. Is it or is it not gcc policy to push a large number of test cases that currently do not work and XFAIL them? Regards Thomas
Hi all, hi Thomas (2x), hi Sandra, On 16.07.21 09:52, Thomas Koenig via Fortran wrote: >> 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. ... On 17.07.21 09:25, Thomas Koenig via Fortran wrote: > Is it or is it not gcc policy to push a large number of test cases > that currently do not work and XFAIL them? In my opinion, it is bad to add testcases which _only_ consist of xfails for 'target *-*-*'; however, for an extensive set of test cases, I think it is better to xfail missing parts than to comment them out - or not having them at all. That permits a better test coverage once the features have been implemented. For the TS29113 patch, which Sandra has posted on July 7, I count: * 77 'dg-do run' tests - of which 27 are xfailed (35%) * 28 compile-time tests * 291 dg-error - of which 59 are xfailed (20%) * 29 dg-bogus - of which are 25 are xfailed (86%) (And of course, those lines which are valid do not have a dg-error - and usually also no dg-bogus.) And in total: * 1 '.exp' file * 105 '.f90' files (with 8232 lines in total including comment lines) * 53 '.c'files (5281 lines) * 1 '.h' file (12 lines) Hence, for me this sounds a rather reasonable amount of xfail. Especially, given that several pending patches do/will reduce the amount of xfails by fixing issues exposed by the testsuite (which has been posted but so far not reviewed). Of course, in an ideal world, xfail would not exist :-) On 07.07.21 05:40, Sandra Loosemore wrote: > There was a question in one of the issues about why this testsuite > references TS29113 instead of the 2018 standard. Well, that is what > our customer is interested in: finding out what parts of the TS29113 > functionality remain unimplemented or broken, and fixing them, so that > gfortran can say that it implements that specification. I believe the only real difference between TS29113 and Fortran 2018's interoperability support is that 'select rank' was added in Fortran 2018. The testsuite also tests 'select rank'; in that sense, it is also for Fortran 2018. Thus, ts29113 + ts29113.exp or 'f2018-c-interop' + 'f2018-c-interop.exp' are both fine to me. — 'ts29113' is shorter while the other is clearer to those who did not follow the Fortran standards and missed that there was a technical specification (TS) between F2008 and F2018, incorporated (with tiny modifications) in F2018. Tobias ----------------- 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
On 16.07.21 05:46, Sandra Loosemore wrote: > When I was reading code in conjunction with fixing PR101317, I noticed > an unrelated bug in the implementation of CFI_allocate and > CFI_select_part: they were mis-handling the CFI_signed_char type as > if it were a Fortran character type for the purposes of deciding > whether to use the elem_len argument to those functions. It's really > an integer type that has the size of signed char. I checked similar > code in other functions in ISO_Fortran_binding.c and these were the > only two that were incorrect. I think there was at least one other place, but that one has been fixed in the meanwhile, missing the other two occurrences you found. > Bind(c): signed char is not a Fortran character type > > CFI_allocate and CFI_select_part were incorrectly treating > CFI_type_signed_char as a Fortran character type for the purpose of > deciding whether or not to use the elem_len argument. It is a Fortran > integer type per table 18.2 in the 2018 Fortran standard. > > Other functions in ISO_Fortran_binding.c appeared to handle this case > correctly already. > > 2021-07-15 Sandra Loosemore<sandra@codesourcery.com> > > gcc/testsuite/ > * gfortran.dg/ts29113/library/allocate-c.c (ctest): Also test > handling of elem_len for CFI_type_char vs CFI_type_signed_char. > * gfortran.dg/ts29113/library/select-c.c (ctest): Likewise. > > libgfortran/ > * runtime/ISO_Fortran_binding.c (CFI_allocate) LGTM. Thanks! Tobias ----------------- 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
commit 45190d9eb5123df77bd60a1d6712f05a3af5f42c
Author: Sandra Loosemore <sandra@codesourcery.com>
Date: Thu Jul 15 16:51:55 2021 -0700
Bind(c): signed char is not a Fortran character type
CFI_allocate and CFI_select_part were incorrectly treating
CFI_type_signed_char as a Fortran character type for the purpose of
deciding whether or not to use the elem_len argument. It is a Fortran
integer type per table 18.2 in the 2018 Fortran standard.
Other functions in ISO_Fortran_binding.c appeared to handle this case
correctly already.
2021-07-15 Sandra Loosemore <sandra@codesourcery.com>
gcc/testsuite/
* gfortran.dg/ts29113/library/allocate-c.c (ctest): Also test
handling of elem_len for CFI_type_char vs CFI_type_signed_char.
* gfortran.dg/ts29113/library/select-c.c (ctest): Likewise.
libgfortran/
* runtime/ISO_Fortran_binding.c (CFI_allocate)
diff --git a/gcc/testsuite/gfortran.dg/ts29113/library/allocate-c.c b/gcc/testsuite/gfortran.dg/ts29113/library/allocate-c.c
index 0208e5a..6343d28 100644
--- a/gcc/testsuite/gfortran.dg/ts29113/library/allocate-c.c
+++ b/gcc/testsuite/gfortran.dg/ts29113/library/allocate-c.c
@@ -135,5 +135,34 @@ ctest (void)
CFI_deallocate (dv));
if (dv->base_addr != NULL)
abort ();
+
+ /* Signed char is not a Fortran character type. Here we expect it to
+ ignore the elem_len argument and use the size of the type. */
+ ex[0] = 3;
+ ex[1] = 4;
+ ex[2] = 5;
+ check_CFI_status ("CFI_establish",
+ CFI_establish (dv, NULL, CFI_attribute_allocatable,
+ CFI_type_signed_char, 4, 3, ex));
+ lb[0] = 1;
+ lb[1] = 2;
+ lb[2] = 3;
+ ub[0] = 10;
+ ub[1] = 5;
+ ub[2] = 10;
+ sm = sizeof (double);
+ check_CFI_status ("CFI_allocate",
+ CFI_allocate (dv, lb, ub, sm));
+ dump_CFI_cdesc_t (dv);
+ if (dv->base_addr == NULL)
+ abort ();
+ if (dv->elem_len != sizeof (signed char))
+ abort ();
+
+ check_CFI_status ("CFI_deallocate",
+ CFI_deallocate (dv));
+ if (dv->base_addr != NULL)
+ abort ();
+
}
diff --git a/gcc/testsuite/gfortran.dg/ts29113/library/select-c.c b/gcc/testsuite/gfortran.dg/ts29113/library/select-c.c
index df6172c..9bcbc01 100644
--- a/gcc/testsuite/gfortran.dg/ts29113/library/select-c.c
+++ b/gcc/testsuite/gfortran.dg/ts29113/library/select-c.c
@@ -8,6 +8,8 @@
/* Declare some source arrays. */
struct ss {
+ char c[4];
+ signed char b[4];
int i, j, k;
} s[10][5][3];
@@ -61,6 +63,31 @@ ctest (void)
if (result->dim[2].sm != source->dim[2].sm)
abort ();
+ /* Check that we use the given elem_size for char but not for
+ signed char, which is considered an integer type instead of a Fortran
+ character type. */
+ check_CFI_status ("CFI_establish",
+ CFI_establish (result, NULL, CFI_attribute_pointer,
+ CFI_type_char, 4, 3, NULL));
+ if (result->elem_len != 4)
+ abort ();
+ offset = offsetof (struct ss, c);
+ check_CFI_status ("CFI_select_part",
+ CFI_select_part (result, source, offset, 4));
+ if (result->elem_len != 4)
+ abort ();
+
+ check_CFI_status ("CFI_establish",
+ CFI_establish (result, NULL, CFI_attribute_pointer,
+ CFI_type_signed_char, 4, 3, NULL));
+ if (result->elem_len != sizeof (signed char))
+ abort ();
+ offset = offsetof (struct ss, c);
+ check_CFI_status ("CFI_select_part",
+ CFI_select_part (result, source, offset, 4));
+ if (result->elem_len != sizeof (signed char))
+ abort ();
+
/* Extract an array of character substrings. */
offset = 2;
check_CFI_status ("CFI_establish",
diff --git a/libgfortran/runtime/ISO_Fortran_binding.c b/libgfortran/runtime/ISO_Fortran_binding.c
index 78953d0..9fe3a85 100644
--- a/libgfortran/runtime/ISO_Fortran_binding.c
+++ b/libgfortran/runtime/ISO_Fortran_binding.c
@@ -229,10 +229,9 @@ CFI_allocate (CFI_cdesc_t *dv, const CFI_index_t lower_bounds[],
}
}
- /* If the type is a character, 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->type == CFI_type_signed_char)
+ /* 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;
/* Dimension information and calculating the array length. */
@@ -759,9 +758,9 @@ int CFI_select_part (CFI_cdesc_t *result, const CFI_cdesc_t *source,
}
}
- /* Element length. */
- if (result->type == CFI_type_char || result->type == CFI_type_ucs4_char ||
- result->type == CFI_type_signed_char)
+ /* Element length is ignored unless result->type specifies a Fortran
+ character type. */
+ if (result->type == CFI_type_char || result->type == CFI_type_ucs4_char)
result->elem_len = elem_len;
if (unlikely (compile_options.bounds_check))