| Message ID | 40382b56-b0e7-a0e6-8911-081a38f58a7a@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fortran: Fix character-kind=4 substring resolution (PR95837) | expand |
Ups – old patch :-( Correct one attached. Tobias On 6/23/20 11:41 PM, Tobias Burnus wrote: > Found when looking at another issue … > > OK for the trunk? > > Tobias > > PS: Without the patch, it fails to compile with: > Error: Character ‘\U0001F600’ in string at (1) cannot be converted > into character kind 1 > Error: Operands of comparison operator ‘/=’ at (1) are > CHARACTER(3)/CHARACTER(3,4) > ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Hi Tobias,
> OK for the trunk?
I just checked, and this gets a segfault for
program main
character (len=3), parameter :: x = 'abc'
print *, x(2:2)
end program main
+ if (ts)
+ e->kind = ts->kind;
+ else if (e->symtree->n.sym->ts.type == BT_CHARACTER)
+ e->kind = ts->kind;
There are two potential problems: e->symtree could be NULL,
and (if the second assignment is reached) ts is NULL.
You may have meant
e->kind = e->symtree->n.sym->ts.kind;
Could you correct that, and resubmit?
Thanks for working on this!
Best regards
Thomas
Hi Thomas, could you review the second patch instead? I have sent the wrong patch (early draft) and corrected it half an hour later! https://gcc.gnu.org/pipermail/gcc-patches/2020-June/548779.html Tobias On 6/24/20 8:01 PM, Thomas Koenig via Fortran wrote: > Hi Tobias, > >> OK for the trunk? > > I just checked, and this gets a segfault for > > program main > character (len=3), parameter :: x = 'abc' > print *, x(2:2) > end program main > > > + if (ts) > + e->kind = ts->kind; > + else if (e->symtree->n.sym->ts.type == BT_CHARACTER) > + e->kind = ts->kind; > > There are two potential problems: e->symtree could be NULL, > and (if the second assignment is reached) ts is NULL. > You may have meant > > e->kind = e->symtree->n.sym->ts.kind; > > Could you correct that, and resubmit? > > Thanks for working on this! > > Best regards > > Thomas
Hi Tobias, > could you review the second patch instead? I have sent the wrong patch > (early draft) and corrected it half an hour later! Sorry, I missed that. Here's the review of the real patch :-) So, the first part is + if (ts) + e->ts.kind = ts->kind; Ok, I unerstand that - ts has been set earlier for a component. But this part + else if (e->ts.type != BT_CHARACTER) + e->ts.kind = gfc_default_character_kind; I do not quite understand. How can the type of an expression involving a substring not be BT_CHARACTER when gfc_resolve_substring_charlen is called? Or is it BT_UNKNOWN, and a check for that might be better? And, if it is indeed BT_UNKNOWN, how do we know it isn't a CHARACTER(KIND=4)? Best Regards Thomas
Hi Thomas, Updated patch – now having submitted the mapping patch, I can focus on other things, including this patch. I have now removed the setting of the typespec – and added an assert to be sure everything is consistent. At least for the testsuite, it is. Thanks for insisting on doing it properly. It was clear that the previous code was wrong – but it was not obvious whether updating the kind value was ever useful. However, testing indicates that the expression-type resolution works and the answer is "never". Thus, an alternative patch would be to just remove the e-> ... = ... without adding an assert. OK – with assert or without? Tobias On 6/24/20 10:02 PM, Thomas Koenig wrote: > Hi Tobias, > >> could you review the second patch instead? I have sent the wrong >> patch (early draft) and corrected it half an hour later! > > Sorry, I missed that. Here's the review of the real patch :-) > > So, the first part is > > + if (ts) > + e->ts.kind = ts->kind; > > Ok, I unerstand that - ts has been set earlier for a component. > > But this part > > + else if (e->ts.type != BT_CHARACTER) > + e->ts.kind = gfc_default_character_kind; > > I do not quite understand. How can the type of an expression involving > a substring not be BT_CHARACTER when gfc_resolve_substring_charlen is > called? Or is it BT_UNKNOWN, and a check for that might be better? > And, if it is indeed BT_UNKNOWN, how do we know it isn't a > CHARACTER(KIND=4)? > > Best Regards > > Thomas > > ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Hi Tobias,
> OK – with assert or without?
I don't think the assert is needed - if things go wrong there, then
I am quite sure that we will get all sorts of ICEs downstream if
this is not set correctly somewhere after all, and your testing
indicates that it is not.
Besides, I like the elegance of fixing a bug by just removing erroneous
code :-)
So, OK without the assert.
Thanks for the patch!
Best regards
Thomas
Fortran: Fix character-kind=4 substring resolution (PR95837) gcc/fortran/ChangeLog: PR fortran/95837 * resolve.c (gfc_resolve_substring_charlen): Fix char-kind setting. gcc/testsuite/ChangeLog: PR fortran/95837 * gfortran.dg/char4-subscript.f90: New test. gcc/fortran/resolve.c | 7 ++++++- gcc/testsuite/gfortran.dg/char4-subscript.f90 | 30 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index c53b312f7ed..6d844dd2310 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -5141,7 +5141,12 @@ gfc_resolve_substring_charlen (gfc_expr *e) } e->ts.type = BT_CHARACTER; - e->ts.kind = gfc_default_character_kind; + if (ts) + e->kind = ts->kind; + else if (e->symtree->n.sym->ts.type == BT_CHARACTER) + e->kind = ts->kind; + else + e->kind = gfc_default_character_kind; if (!e->ts.u.cl) e->ts.u.cl = gfc_new_charlen (gfc_current_ns, NULL); diff --git a/gcc/testsuite/gfortran.dg/char4-subscript.f90 b/gcc/testsuite/gfortran.dg/char4-subscript.f90 new file mode 100644 index 00000000000..f1f915c7af9 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/char4-subscript.f90 @@ -0,0 +1,30 @@ +! { dg-do run } +! { dg-additional-options "-fdump-tree-original" } +! +! PR fortran/95837 +! +type t + character(len=:, kind=4), pointer :: str2 +end type t +type(t) :: var + +allocate(character(len=5, kind=4) :: var%str2) + +var%str2(1:1) = 4_"d" +var%str2(2:3) = 4_"ef" +var%str2(4:4) = achar(int(Z'1F600'), kind=4) +var%str2(5:5) = achar(int(Z'1F608'), kind=4) + +if (var%str2(1:3) /= 4_"def") stop 1 +if (ichar(var%str2(4:4)) /= int(Z'1F600')) stop 2 +if (ichar(var%str2(5:5)) /= int(Z'1F608')) stop 2 + +deallocate(var%str2) +end + +! Note: the last '\x00' is regarded as string terminator, hence, the tailing \0 byte is not in the dump + +! { dg-final { scan-tree-dump " \\(\\*var\\.str2\\)\\\[1\\\]{lb: 1 sz: 4} = .d\\\\x00\\\\x00.\\\[1\\\]{lb: 1 sz: 4};" "original" } } +! { dg-final { scan-tree-dump " __builtin_memmove \\(\\(void \\*\\) &\\(\\*var.str2\\)\\\[2\\\]{lb: 1 sz: 4}, \\(void \\*\\) &.e\\\\x00\\\\x00\\\\x00f\\\\x00\\\\x00.\\\[1\\\]{lb: 1 sz: 4}, 8\\);" "original" } } +! { dg-final { scan-tree-dump " \\(\\*var.str2\\)\\\[4\\\]{lb: 1 sz: 4} = .\\\\x00\\\\xf6\\\\x01.\\\[1\\\]{lb: 1 sz: 4};" "original" } } +! { dg-final { scan-tree-dump " \\(\\*var.str2\\)\\\[5\\\]{lb: 1 sz: 4} = .\\\\b\\\\xf6\\\\x01.\\\[1\\\]{lb: 1 sz: 4};" "original" } }