This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR 78534 Change character length from int to size_t


On Fri, Dec 16, 2016 at 5:34 PM, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi Janne, hi all,
>
> as promised please find attached the change from int32 to size_t for the
> caf-libraries. Because the caf-libraries do not require special notions
> indicated by negative values I went for using size_t there. I assume this will
> be easier to keep in sync for all caf-libraries, because the size_t is
> available on all modern platforms. I also took the liberty to fix the
> specifiers in trans-decl for the caf-function-declarations and update the
> documentation on the caf-functions in gfortran.texi where some parameters where
> missing.
>
> These additional changes bootstrap fine and induce no new regressions on
> x86_64-linux/f23.

I think you should use build_zero_cst(size_type_node) instead of
size_zero_node as size_zero_node is of type sizetype which is not the
same as size_type_node. Otherwise looks good.

And yes, I think it makes sense to use size_t directly instead of
introducing the GFortran specific gfc_charlen_type typedef.

> I am still not sure, whether we shouldn't address the regression in
> allocate_deferred_char_scalar_1. I did some research, but could not yet come to
> a practical solution. The backend somehow deduces that the memory pointed to
> has size 5 only. But I haven't found a way to do this in the front end.
>
> Comments on these changes?
>
> - Andre
>
> On Tue, 13 Dec 2016 21:08:51 +0200
> Janne Blomqvist <blomqvist.janne@gmail.com> wrote:
>
>> On Mon, Dec 12, 2016 at 9:39 PM, Janne Blomqvist
>> <blomqvist.janne@gmail.com> wrote:
>> > On Mon, Dec 12, 2016 at 7:39 PM, Andre Vehreschild <vehre@gmx.de> wrote:
>> >> Hi Janne,
>> >>
>> >> I found that you are favoring build_int_cst (size_type_node, 0) over
>> >> size_zero_node. Is there a reason to this?
>> >
>> > Yes. AFAIU size_zero_node is a zero constant for sizetype which is not
>> > the same as size_type_node.
>> >
>> > AFAIK the difference is that size_type_node is the C size_t type,
>> > whereas sizetype is a GCC internal type used for address expressions.
>> > On a "normal" target I understand that they are the same size, but
>> > there are some slight differences in semantics, e.g. size_type_node
>> > like C unsigned integer types is defined to wrap around on overflow
>> > whereas sizetype is undefined on overflow.
>> >
>> > I don't know if GCC supports some strange targets with some kind of
>> > segmented memory where the size of sizetype would be different from
>> > size_type_node, but I guess it's possible in theory at least.
>> >
>> > That being said, now that you mention in I should be using
>> > build_zero_cst (some_type_node) instead of
>> > build_int_cst(some_type_node, 0). There's also build_one_cst that I
>> > should use.
>> >
>> >> Furthermore did I have to patch this:
>> >>
>> >> diff --git a/gcc/fortran/dump-parse-tree.c b/gcc/fortran/dump-parse-tree.c
>> >> index 585f25d..f374558 100644
>> >> --- a/gcc/fortran/dump-parse-tree.c
>> >> +++ b/gcc/fortran/dump-parse-tree.c
>> >> @@ -465,7 +465,7 @@ show_expr (gfc_expr *p)
>> >>           break;
>> >>
>> >>         case BT_HOLLERITH:
>> >> -         fprintf (dumpfile, "%dH", p->representation.length);
>> >> +         fprintf (dumpfile, "%zdH", p->representation.length);
>> >>           c = p->representation.string;
>> >>           for (i = 0; i < p->representation.length; i++, c++)
>> >>             {
>> >>
>> >> to bootstrap on x86_64-linux/f23.
>> >
>> > Ah, thanks for the catch. I'll fix it by using HOST_WIDE_INT_PRINT_DEC
>> > since I'll have to change gfc_charlen_t to be a typedef form
>> > HOST_WIDE_INT (see my answer to FX).
>> >
>> >> And I have this regression:
>> >>
>> >> FAIL: gfortran.dg/allocate_deferred_char_scalar_1.f03   -O1  (test for
>> >> excess errors)
>> >>
>> >> allocate_deferred_char_scalar_1.f03:184:0:
>> >>
>> >>      p = '12345679'
>> >>
>> >> Warning: '__builtin_memcpy' writing 8 bytes into a region of size 5
>> >> overflows the destination [-Wstringop-overflow=]
>> >>      allocate_deferred_char_scalar_1.f03:242:0:
>> >>
>> >>      p = 4_'12345679'
>> >>
>> >> Warning: '__builtin_memcpy' writing 32 bytes into a region of size 20
>> >> overflows the destination [-Wstringop-overflow=]
>> >
>> > I'm seeing that too, but I assumed they would be fixed by Paul's
>> > recent patch which I don't yet have in my tree yet due to the git
>> > mirror being stuck..
>> >
>> >> Btw, the patch for changing the ABI of the coarray-libs is already nearly
>> >> done. I just need to figure that what the state of regressions is with and
>> >> without my change.
>> >
>> > Thanks.
>> >
>> > I'll produce an updated patch with the changes discussed so far.
>> >
>> >
>> > --
>> > Janne Blomqvist
>>
>> Hi,
>>
>> attached is the updated patch that applies on top of the original. I
>> didn't do the charlen_zero_node etc, I just fixed the relatively few
>> places in my previous patch rather than everywhere in the entire
>> frontend.
>>
>> Now that the git mirror is working again, I see though those warnings
>> with -O1 from gfortran.dg/allocate_deferred_char_scalar_1.f03 are
>> still there, and Paul's patch didn't get rid of them. :(. I have
>> looked at the tree dumps, however, and AFAICS it's a bogus warning.
>>
>> Ok for trunk?
>>
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de



-- 
Janne Blomqvist


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]