Update: [patch, libfortran] Fix EOF handling in array I/O
Here's an update to the previous patch.
Upon reflection, I think it is better for performance to have two
versions of the loop so the test is only performed when it is
needed.
So, OK for trunk?
Regards
Thomas
Fix EOF handling for arrays.
2019-11-23 Thomas Koenig
Harald Anlauf
PR fortran/92569
* io/transfer.c (transfer_array_inner): If position is
at AFTER_ENDFILE in current unit, return from data loop.
2019-11-23 Thomas Koenig
Harald Anlauf
PR fortran/92569
* gfortran.dg/eof_4.f90: New test.
Index: io/transfer.c
===
--- io/transfer.c (Revision 278025)
+++ io/transfer.c (Arbeitskopie)
@@ -2542,26 +2542,62 @@ transfer_array_inner (st_parameter_dt *dtp, gfc_ar
data = GFC_DESCRIPTOR_DATA (desc);
- while (data)
+ /* When reading, we need to check endfile conditions so we do not miss
+ an END=label. Make this separate so we do not have an extra test
+ in a tight loop when it is not needed. */
+
+ if (dtp->u.p.current_unit && dtp->u.p.mode == READING)
{
- dtp->u.p.transfer (dtp, iotype, data, kind, size, tsize);
- data += stride0 * tsize;
- count[0] += tsize;
- n = 0;
- while (count[n] == extent[n])
+ while (data)
{
- count[n] = 0;
- data -= stride[n] * extent[n];
- n++;
- if (n == rank)
+ if (unlikely (dtp->u.p.current_unit->endfile == AFTER_ENDFILE))
+ return;
+
+ dtp->u.p.transfer (dtp, iotype, data, kind, size, tsize);
+ data += stride0 * tsize;
+ count[0] += tsize;
+ n = 0;
+ while (count[n] == extent[n])
{
- data = NULL;
- break;
+ count[n] = 0;
+ data -= stride[n] * extent[n];
+ n++;
+ if (n == rank)
+ {
+ data = NULL;
+ break;
+ }
+ else
+ {
+ count[n]++;
+ data += stride[n];
+ }
}
- else
+ }
+}
+ else
+{
+ while (data)
+ {
+ dtp->u.p.transfer (dtp, iotype, data, kind, size, tsize);
+ data += stride0 * tsize;
+ count[0] += tsize;
+ n = 0;
+ while (count[n] == extent[n])
{
- count[n]++;
- data += stride[n];
+ count[n] = 0;
+ data -= stride[n] * extent[n];
+ n++;
+ if (n == rank)
+ {
+ data = NULL;
+ break;
+ }
+ else
+ {
+ count[n]++;
+ data += stride[n];
+ }
}
}
}
! { dg-do run }
! { dg-options "-ffrontend-optimize" }
! PR 92569 - the EOF condition was not recognized with
! -ffrontend-optimize. Originjal test case by Bill Lipa.
program main
implicit none
real(kind=8) :: tdat(1000,10)
real(kind=8) :: res (10, 3)
integer :: i, j, k, np
open (unit=20, status="scratch")
res = reshape([(real(i),i=1,30)], shape(res))
write (20,'(10G12.5)') res
rewind 20
do j = 1,1000
read (20,*,end=1)(tdat(j,k),k=1,10)
end do
1 continue
np = j-1
if (np /= 3) stop 1
if (any(transpose(res) /= tdat(1:np,:))) stop 2
end program main
Re: [PATCH ix86] Fix rtx_costs for flag-setting adds
On 11/22/19 6:05 PM, Uros Bizjak wrote:
> Indeed, this is a different case, an overflow test that results in one
> CMP insn. I think, we should check if the second operand is either 0
> (then proceed as it is now), or if the second operand equals first
> operand of PLUS insn, then we actually emit CMP insn (please see
> PR30315).
Here's what I committed - preapproved by Uros off-list (I forgot
Reply-All again).
Bernd
Index: gcc/ChangeLog
===
--- gcc/ChangeLog (revision 278653)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2019-11-24 Bernd Schmidt
+
+ * config/i386/i386.c (ix86_rtx_costs): Handle care of a PLUS in a
+ COMPARE, representing an overflow detection.
+
2019-11-23 Jan Hubicka
* cif-code.def (MAX_INLINE_INSNS_SINGLE_O2_LIMIT): Remove.
Index: gcc/config/i386/i386.c
===
--- gcc/config/i386/i386.c (revision 278653)
+++ gcc/config/i386/i386.c (working copy)
@@ -19501,6 +19501,15 @@ ix86_rtx_costs (rtx x, machine_mode mode
return true;
}
+ if (GET_CODE (XEXP (x, 0)) == PLUS
+ && rtx_equal_p (XEXP (XEXP (x, 0), 0), XEXP (x, 1)))
+ {
+ /* This is an overflow detection, count it as a normal compare. */
+ *total = rtx_cost (XEXP (x, 0), GET_MODE (XEXP (x, 0)),
+ COMPARE, 0, speed);
+ return true;
+ }
+
/* The embedded comparison operand is completely free. */
if (!general_operand (XEXP (x, 0), GET_MODE (XEXP (x, 0)))
&& XEXP (x, 1) == const0_rtx)
Re: [PATCH 4/4] Fix autoinc cbranch
On 11/19/19 1:27 AM, Segher Boessenkool wrote: > The combine parts are okay for trunk, if you keep an eye out :-) Thanks, now committed. That leaves the auto-inc-dec part. Since we're being adventurous, I've also bootstrapped and tested the following in the meantime (on the gcc135 machine). This just deletes the tests for jumps entirely rather than hedging bets. Bernd Index: gcc/auto-inc-dec.c === --- gcc/auto-inc-dec.c (revision 278653) +++ gcc/auto-inc-dec.c (working copy) @@ -1441,12 +1441,6 @@ merge_in_block (int max_reg, basic_block continue; } - /* This continue is deliberate. We do not want the uses of the - jump put into reg_next_use because it is not considered safe to - combine a preincrement with a jump. */ - if (JUMP_P (insn)) - continue; - if (dump_file) dump_insn_slim (dump_file, insn);
Re: [PATCH 1/4] MSP430: Disable TM clone registry by default
On Sun, 17 Nov 2019 12:11:23 -0700 Jeff Law wrote: > On 11/7/19 2:34 PM, Jozef Lawrynowicz wrote: > > Given that MSP430 is a resource constrained, embedded target disabling > > transactional memory by default is a good idea to save on code size in > > the runtime library. > > > > It can still be enabled by passing --enable-tm-clone-registry (although as > > far > > as I understand the feature is fundamentally incompatible with MSP430 given > > reliance on libitm, lack of thread support without an OS and the memory > > limitations of the device. > > > I'm not a huge fan of making the default configurations behave > differently. But I can also see how something like TM in particular > isn't of much interest in the embedded space (hell, it's having trouble > getting real traction in the server space as well). > > May be a reasonable path forward is to add the configury bits, keep TM > on by default and create a different msp target which disables this stuff? Ok fair enough, what would an acceptable form for a new target triplet look like? "msp430-*-elfbare"? Since we're into stage 3 now I'll look at doing this for GCC 11. Thanks, Jozef > > Jeff > > ps. I thought libitm would fallback to a full software solution and the > hardware requirements were really just enabling fast-paths. >
Re: [PATCH 4/4] MSP430: Deprecate -minrt option
On Sun, 17 Nov 2019 14:00:58 -0700
Jeff Law wrote:
> On 11/7/19 2:41 PM, Jozef Lawrynowicz wrote:
> > Support for the MSP430 -minrt option has been removed from Newlib, since
> > all the
> > associated behaviour is now dynamic. Initialization code run before main is
> > only
> > included when needed.
> >
> > This patch removes the final traces of -minrt from GCC.
> >
> > -minrt used to modify the linking process in the following ways:
> > * Removing .init and .fini sections, by using a reduced crt0 and excluding
> > crtn.
> > * Removing crtbegin and crtend (thereby not using crtstuff.c at all).
> > + This meant that even if the program had constructors for global or
> > static objects which must run before main, it would blindly remove them.
> >
> > These causes of code bloat have been addressed by:
> > * switching to .{init,fini}_array instead of using .{init,fini} sections
> > "Lean" code to run through constructors before main is only included if
> > .init_array has contents.
> > * removing bloat (frame_dummy, *tm_clones*, *do_global_dtors*) from the
> > crtstuff.c with the changes in the previous patches
> >
> > Here are some examples of the total size of different "barebones" C
> > programs to
> > show that the size previously achieved by -minrt is now matched by default:
> >
> > program |old (with -minrt) |new (without -minrt)
> > -
> > Empty main |20 |20
> > Looping main|14 |14
> > Looping main with data |94 |94
> > Looping main with bss |56 |56
> >
> >
> > 0004-MSP430-Remove-minrt-option.patch
> >
> > From 6e561b45c118540f06d5828ec386d2dd79c13b62 Mon Sep 17 00:00:00 2001
> > From: Jozef Lawrynowicz
> > Date: Wed, 6 Nov 2019 18:12:45 +
> > Subject: [PATCH 4/4] MSP430: Remove -minrt option
> >
> > gcc/ChangeLog:
> >
> > 2019-11-07 Jozef Lawrynowicz
> >
> > * config/msp430/msp430.h (STARTFILE_SPEC): Remove -minrt rules.
> > Use "if, then, else" syntax for specs.
> > (ENDFILE_SPEC): Likewise.
> > * config/msp430/msp430.opt: Mark -minrt as deprecated.
> > * doc/invoke.texi: Remove -minrt documentation.
> This is fine. I leave the decision whether or not to install now or
> wait for resolution on the other changes in this space as your decision.
Unfortunately without the other changes installed by default this option is
still required if users want the bare minimum code size.
Jozef
>
> jeff
>
Re: Convert inliner to new param infrastructure
Hi Jan, > This patch adds opt_for_fn for all cross module params used by inliner > so they can be modified at function granuality. With inlining almost always > there are three functions to consider (callee and caller of the inlined edge > and the outer function caller is inlined to). > > I always use the outer function params since that is how local parameters > behave. I hope it is kind of what is also expected in most case: it is better > to inline agressively into -O3 compiled code rather than inline agressively > -O3 > functions into their callers. > > New params infrastructure is nice. One drawback is that is very hard to > search for individual param uses since they all occupy global namespace. > With C++ world we had chance to do something like params.param_flag_name > or params::param_flag_name instead... > > Bootstrapped/regtested x86_64-linux, comitted. this patch introduced a regression (everywhere, it seems): +FAIL: gcc.dg/winline-3.c (test for warnings, line 5) +FAIL: gcc.dg/winline-3.c (test for excess errors) Excess errors: /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/winline-3.c:5:12: warning: inlining failed in call to 'q': --param max-inline-insns-auto limit reached [-Winline] I suspect you just forgot to adapt the expected warning message ((...-single -> ...-auto)? Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
[patch, fortran] Fix PR 91783
Hello world,
this patch fixes a 10 regression in dependency checking. The
approach is simple - if gfc_dep_resolver is handed references
with _data, remove that.
Regression-tested. OK for trunk?
Regards
Thomas
Do not look at _data component in gfc_dep_resolver.
2019-11-24 Thomas Koenig
PR fortran/91783
* dependency.c (gfc_dep_resolver): Do not look at _data
component if present.
2019-11-24 Thomas Koenig
PR fortran/91783
* gfortran.dg/dependency_56.f90: New test.
Index: fortran/dependency.c
===
--- fortran/dependency.c (Revision 278025)
+++ fortran/dependency.c (Arbeitskopie)
@@ -2098,6 +2098,18 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf
gfc_dependency this_dep;
bool same_component = false;
+ /* The refs might come in mixed, one with a _data component and one
+ without. Look at their next reference in order to avoid an
+ ICE. */
+
+ if (lref && lref->type == REF_COMPONENT && lref->u.c.component
+ && strcmp (lref->u.c.component->name, "_data") == 0)
+lref = lref->next;
+
+ if (rref && rref->type == REF_COMPONENT && rref->u.c.component
+ && strcmp (rref->u.c.component->name, "_data") == 0)
+rref = rref->next;
+
this_dep = GFC_DEP_ERROR;
fin_dep = GFC_DEP_ERROR;
/* Dependencies due to pointers should already have been identified.
! { dg-do compile }
! PR 91783 - used to cause an ICE in dependency checking.
! Test case by Gerhard Steinmetz.
program p
class(*), allocatable :: a(:)
a = [1, 2, 3]
a = f(a)
contains
function f(x) result(y)
class(*), allocatable, intent(in) :: x(:)
class(*), allocatable :: y(:)
y = x
end
end
Re: [PATCH 1/4] MSP430: Disable TM clone registry by default
On 11/24/19 7:20 AM, Jozef Lawrynowicz wrote: > On Sun, 17 Nov 2019 12:11:23 -0700 > Jeff Law wrote: > >> On 11/7/19 2:34 PM, Jozef Lawrynowicz wrote: >>> Given that MSP430 is a resource constrained, embedded target disabling >>> transactional memory by default is a good idea to save on code size in >>> the runtime library. >>> >>> It can still be enabled by passing --enable-tm-clone-registry (although as >>> far >>> as I understand the feature is fundamentally incompatible with MSP430 given >>> reliance on libitm, lack of thread support without an OS and the memory >>> limitations of the device. >>> >> I'm not a huge fan of making the default configurations behave >> differently. But I can also see how something like TM in particular >> isn't of much interest in the embedded space (hell, it's having trouble >> getting real traction in the server space as well). >> >> May be a reasonable path forward is to add the configury bits, keep TM >> on by default and create a different msp target which disables this stuff? > > Ok fair enough, what would an acceptable form for a new target triplet look > like? "msp430-*-elfbare"? Yea, that seems reasonable. > > Since we're into stage 3 now I'll look at doing this for GCC 11. I'd seriously consider letting this into gcc-10. It's going to be well isolated and it's an iteration of something you posted before the stage1->stage3 transition. Your choice if you want to try and pull it together quickly for gcc-10. Jeff
Re: [C++ PATCH] c++/91353 - P1331R2: Allow trivial default init in constexpr contexts.
On 11/16/19 5:23 PM, Marek Polacek wrote: [ Working virtually on Baker Island. ] This patch implements C++20 P1331, allowing trivial default initialization in constexpr contexts. I used Jakub's patch from the PR which allowed uninitialized variables in constexpr contexts. But the hard part was handling CONSTRUCTOR_NO_CLEARING which is always cleared in cxx_eval_call_expression. We need to set it in the case a constexpr constructor doesn't initialize all the members, so that we can give proper diagnostic instead of value-initializing. A lot of my attempts flopped but then I came up with this approach, which handles various cases as tested in constexpr-init8.C, where S is initialized by a non-default constexpr constructor, and constexpr-init9.C, using delegating constructors. And the best part is that I didn't need any new cx_check_missing_mem_inits calls! Just save the information whether a constructor is missing an init into constexpr_fundef_table and retrieve it when needed. Is it necessary to clear the flag for constructors that do happen to initialize all the members? I would think that leaving that clearing to reduced_constant_expression_p would be enough. constexpr-init10.C demonstrates that we can now elide a constructor call, this is caused by the walk_field_subobs hunk. I hope that's OK. So long as the object's vptr is initialized properly, absolutely. Since a has static storage duration, constant zero-initialization plus the default constructor fully initialize the object. Jason
Re: [PATCH 4/4] Fix autoinc cbranch
Hi! On Sun, Nov 24, 2019 at 03:19:49PM +0100, Bernd Schmidt wrote: > - /* This continue is deliberate. We do not want the uses of the > - jump put into reg_next_use because it is not considered safe to > - combine a preincrement with a jump. */ > - if (JUMP_P (insn)) > - continue; Huh, I wonder where that came from. More archaelogy :-) (This patch looks fine to me, fwiw). Segher
Re: [PATCH 1/4] MSP430: Disable TM clone registry by default
On Sun, 24 Nov 2019 10:10:51 -0700 Jeff Law wrote: > On 11/24/19 7:20 AM, Jozef Lawrynowicz wrote: > > On Sun, 17 Nov 2019 12:11:23 -0700 > > Jeff Law wrote: > > > >> On 11/7/19 2:34 PM, Jozef Lawrynowicz wrote: > >>> Given that MSP430 is a resource constrained, embedded target disabling > >>> transactional memory by default is a good idea to save on code size in > >>> the runtime library. > >>> > >>> It can still be enabled by passing --enable-tm-clone-registry (although > >>> as far > >>> as I understand the feature is fundamentally incompatible with MSP430 > >>> given > >>> reliance on libitm, lack of thread support without an OS and the memory > >>> limitations of the device. > >>> > >> I'm not a huge fan of making the default configurations behave > >> differently. But I can also see how something like TM in particular > >> isn't of much interest in the embedded space (hell, it's having trouble > >> getting real traction in the server space as well). > >> > >> May be a reasonable path forward is to add the configury bits, keep TM > >> on by default and create a different msp target which disables this stuff? > >> > > > > Ok fair enough, what would an acceptable form for a new target triplet look > > like? "msp430-*-elfbare"? > Yea, that seems reasonable. > > > > > Since we're into stage 3 now I'll look at doing this for GCC 11. > I'd seriously consider letting this into gcc-10. It's going to be well > isolated and it's an iteration of something you posted before the > stage1->stage3 transition. Your choice if you want to try and pull it > together quickly for gcc-10. Ok great, I will aim to get something together ASAP. Thanks, Jozef > > Jeff >
Re: [PATCH 2/4] The main m68k cc0 conversion
On 11/23/19 12:54 PM, Oleg Endo wrote: > On Sat, 2019-11-23 at 10:36 -0700, Jeff Law wrote: >> >>> Any news on this? I would be in favor of merging these patches as I >>> have >>> tested them successfully on Debian by building the gcc-snapshot >>> package >>> with the patches applied. I used all four patches plus the >>> additional one >>> required to be able to build the kernel. >> >> Not really. I've already indicated to Bernd that he should go ahead >> and >> commit the changes and we can iterate on any problems that arise. >> >>> >>> I did not see the bootstrap problems that Mikael reported and >>> Andreas has >>> reported that the issues is not reliably reproducible on Aranym. He >>> suspected >>> that it might be a bug in the MMU emulation in Aranym which only >>> triggers >>> randomly depending on the current memory layout. >> >> Good to know it wasn't reproducible and might ultimately be a bug in >> aranym. >> > > Meanwhile, another patch that's supposed to do the same thing was > posted: > > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02131.html Yes. I'm aware. Given Bernd's was submitted first and looks generally sound, I think his has priority. jeff
Re: [PATCH] m68k architecture: support ccmode + lra
On 11/21/19 10:30 AM, ste...@franke.ms wrote: > Hi there, > > here is mc68k's patch to switch the m68k architecture over to ccmode and > lra. See https://github.com/mc68kghost/gcc 68k-ccmode branch. Bernd Schmidt posted a conversion of the m68k port to ccmode a couple weeks before yours. We've already ACK'd it for installing onto the trunk. Jeff
Re: [C++ PATCH] Fix concepts vs. PCH (PR c++/92458, take 2)
On 11/22/19 3:08 PM, Jakub Jelinek wrote:
On Fri, Nov 22, 2019 at 02:43:25PM -0500, Jason Merrill wrote:
@@ -5358,6 +5410,18 @@ struct tree_cache_traits
: simple_cache_map_traits, tree> { };
We should probably use tree_hash instead of default_hash_traits here. OK
with or without that change.
Dunno, I'd say pointer_hash ::hash which is
return (hashval_t) ((intptr_t)candidate >> 3);
might be actually better hash function than
#define TREE_HASH(NODE) ((size_t) (NODE) & 077)
at least on 64-bit hosts, because union tree_node is 8-byte aligned there,
so the low 3 bits are all zero and we use all of the 32 bits above that.
For 32-bit hosts, it might be better to just use candidate without shifting
I guess, there aren't any extra bits we get in from above.
TREE_HASH to me unnecessarily uses the low 3 bits known to be zero and masks
with 0x3, so throws away also 14 bits that could hold useful info,
leaving for 64-bit hosts only 15 bits.
True, but that suggests that TREE_HASH should change.
Jason
Re: [PATCH 4/4] Fix autoinc cbranch
On Sun, Nov 24, 2019 at 11:39:05AM -0600, Segher Boessenkool wrote: > On Sun, Nov 24, 2019 at 03:19:49PM +0100, Bernd Schmidt wrote: > > - /* This continue is deliberate. We do not want the uses of the > > -jump put into reg_next_use because it is not considered safe to > > -combine a preincrement with a jump. */ > > - if (JUMP_P (insn)) > > - continue; > > Huh, I wonder where that came from. More archaelogy :-) It came in as r115135 on the dataflow-branch. Patch is at https://gcc.gnu.org/ml/gcc-patches/2006-07/msg00037.html . And that is where I lost track. But. Allowing autoinc into jump insns means those jump insns may then eventually need an output reload; it may just have been because of that? Segher
Re: [PATCH 4/4] Fix autoinc cbranch
On 11/24/19 8:43 PM, Segher Boessenkool wrote: > But. Allowing autoinc into jump insns means those jump insns may then > eventually need an output reload; it may just have been because of that? That's almost certainly the reasoning, but as I pointed out in my original mail - reload is careful around autoincs and emits the reload sequence before the reloaded insn. For example, see inc_for_reload: /* Postincrement. Because this might be a jump insn or a compare, and because RELOADREG may not be available after the insn in an input reload, we must do the incrementation before the insn being reloaded for. On m68k with cc0 this is already necessary, because even a cmp insn cannot have output reloads (they would overwrite the flags). This is why I think it should be safe to allow them in jumps too. Bernd
[Patch,Fortran] PR92629 - ICE in convert_mpz_to_unsigned, at fortran/simplify.c:173
In convert_mpz_to_unsigned, an assert was triggered when a positive
argument larger than HUGE() of the respective kind was passed while
-fno-range-check was specified. The assert should be consistently
skipped in this case.
Regtested on x86_64-pc-linux-gnu.
OK for trunk / 9 / 8?
Thanks,
Harald
2019-11-24 Harald Anlauf
PR fortran/92629
* simplify.c (convert_mpz_to_unsigned): Skip assert for argument
range when -fno-range-check is specified.
2019-11-24 Harald Anlauf
PR fortran/92629
* gfortran.dg/pr92629.f90: New testcase.
Index: gcc/fortran/simplify.c
===
--- gcc/fortran/simplify.c (Revision 278629)
+++ gcc/fortran/simplify.c (Arbeitskopie)
@@ -169,8 +169,10 @@ convert_mpz_to_unsigned (mpz_t x, int bitsize)
}
else
{
- /* Confirm that no bits above the signed range are set. */
- gcc_assert (mpz_scan1 (x, bitsize-1) == ULONG_MAX);
+ /* Confirm that no bits above the signed range are set if we
+are doing range checking. */
+ if (flag_range_check != 0)
+ gcc_assert (mpz_scan1 (x, bitsize-1) == ULONG_MAX);
}
}
Index: gcc/testsuite/gfortran.dg/pr92629.f90
===
--- gcc/testsuite/gfortran.dg/pr92629.f90 (nicht existent)
+++ gcc/testsuite/gfortran.dg/pr92629.f90 (Arbeitskopie)
@@ -0,0 +1,11 @@
+! { dg-do run }
+! { dg-options "-fno-range-check" }
+!
+! Test the fix for PR92629.
+program bge_tests
+ if (bge (huge (1_1), 128_1)) stop 1
+ if (bge (128_1 , 255_1)) stop 2
+ if (bge (huge (1_2), 32768_2)) stop 3
+ if (bge (huge (1_4), 2147483648_4)) stop 4
+ if (bge (huge (1_8), 9223372036854775808_8)) stop 5
+end program
Re: [PATCH 4/4] Fix autoinc cbranch
On Sun, Nov 24, 2019 at 08:55:37PM +0100, Bernd Schmidt wrote: > On 11/24/19 8:43 PM, Segher Boessenkool wrote: > > But. Allowing autoinc into jump insns means those jump insns may then > > eventually need an output reload; it may just have been because of that? > > That's almost certainly the reasoning, but as I pointed out in my > original mail - reload is careful around autoincs and emits the reload > sequence before the reloaded insn. Yes, and that isn't anything new either. Does LRA do this as well? ... Yes it does, see lra-constraints.c, search for "Because this might be a jump" as well. > For example, see inc_for_reload: > /* Postincrement. >Because this might be a jump insn or a compare, and because RELOADREG >may not be available after the insn in an input reload, we must do >the incrementation before the insn being reloaded for. > > On m68k with cc0 this is already necessary, because even a cmp insn > cannot have output reloads (they would overwrite the flags). This is why > I think it should be safe to allow them in jumps too. Yeah, auto-modify makes my head hurt :-) Output reloads definitely are *not* safe in jumps, but auto-modify seems to be indeed. Segher
[patch, committed] PR92100 Formatted stream IO irreproducible read with binary data in file
Committed this as simple one liner. I will probably backport this in a few
days.
Jerry
Committed r278660
M libgfortran/ChangeLog
M libgfortran/io/transfer.c
2019-11-24 Jerry DeLisle
PR fortran/92100
io/transfer.c (data_transfer_init_worker): Use fbuf_reset
instead of fbuf_flush before the seek. Note that fbuf_reset
calls fbuf_flush and adjusts fbuf pointers.
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 6382d0dad09..bb104db3584 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -3273,8 +3273,9 @@ data_transfer_init_worker (st_parameter_dt *dtp, int
read_flag)
if (dtp->pos != dtp->u.p.current_unit->strm_pos)
{
- fbuf_flush (dtp->u.p.current_unit, dtp->u.p.mode);
- if (sseek (dtp->u.p.current_unit->s, dtp->pos - 1, SEEK_SET) < 0)
+ fbuf_reset (dtp->u.p.current_unit);
+ if (sseek (dtp->u.p.current_unit->s, dtp->pos - 1,
+SEEK_SET) < 0)
{
generate_error (&dtp->common, LIBERROR_OS, NULL);
return;
Re: [PATCH] Fix attribute access issues
On 11/22/19 6:03 PM, Jakub Jelinek wrote:
Hi!
On Thu, Nov 21, 2019 at 06:09:34PM -0700, Martin Sebor wrote:
PR middle-end/83859
* c-attribs.c (handle_access_attribute): New function.
(c_common_attribute_table): Add new attribute.
(get_argument_type): New function.
(append_access_attrs): New function.
I'm getting
+FAIL: gcc.dg/Wstringop-overflow-24.c (internal compiler error)
+FAIL: gcc.dg/Wstringop-overflow-24.c (test for excess errors)
on i686-linux, while it succeeds on x86_64-linux. On a closer look,
there is a buffer overflow even on x86_64-linux as can be seen under
valgrind, plus memory leak.
The buffer overflow is in append_access_attrs:
==9759== Command: ./cc1 -quiet -Wall Wstringop-overflow-24.c
==9759==
==9759== Invalid write of size 1
==9759==at 0x483BD9F: strcpy (vg_replace_strmem.c:513)
==9759==by 0xA11FF4: append_access_attrs(tree_node*, tree_node*, char
const*, char, long*) (c-attribs.c:3934)
==9759==by 0xA12AD3: handle_access_attribute(tree_node**, tree_node*,
tree_node*, int, bool*) (c-attribs.c:4158)
==9759==by 0x88E1BF: decl_attributes(tree_node**, tree_node*, int,
tree_node*) (attribs.c:728)
==9759==by 0x8A6A9B: c_decl_attributes(tree_node**, tree_node*, int)
(c-decl.c:4944)
==9759==by 0x8A6FE2: start_decl(c_declarator*, c_declspecs*, bool,
tree_node*) (c-decl.c:5083)
==9759==by 0x91CB15: c_parser_declaration_or_fndef(c_parser*, bool, bool, bool,
bool, bool, tree_node**, vec, bool, tree_node*,
oacc_routine_data*, bool*) (c-parser.c:2216)
==9759==by 0x91B742: c_parser_external_declaration(c_parser*)
(c-parser.c:1690)
==9759==by 0x91B25E: c_parser_translation_unit(c_parser*) (c-parser.c:1563)
==9759==by 0x9590A4: c_parse_file() (c-parser.c:21524)
==9759==by 0x9E308E: c_common_parse_file() (c-opts.c:1185)
==9759==by 0x1211AEE: compile_file() (toplev.c:458)
==9759== Address 0x5113f68 is 0 bytes after a block of size 8 alloc'd
==9759==at 0x483880B: malloc (vg_replace_malloc.c:309)
==9759==by 0x229BF17: xmalloc (xmalloc.c:147)
==9759==by 0xA11FC0: append_access_attrs(tree_node*, tree_node*, char
const*, char, long*) (c-attribs.c:3932)
==9759==by 0xA12AD3: handle_access_attribute(tree_node**, tree_node*,
tree_node*, int, bool*) (c-attribs.c:4158)
==9759==by 0x88E1BF: decl_attributes(tree_node**, tree_node*, int,
tree_node*) (attribs.c:728)
==9759==by 0x8A6A9B: c_decl_attributes(tree_node**, tree_node*, int)
(c-decl.c:4944)
==9759==by 0x8A6FE2: start_decl(c_declarator*, c_declspecs*, bool,
tree_node*) (c-decl.c:5083)
==9759==by 0x91CB15: c_parser_declaration_or_fndef(c_parser*, bool, bool, bool,
bool, bool, tree_node**, vec, bool, tree_node*,
oacc_routine_data*, bool*) (c-parser.c:2216)
==9759==by 0x91B742: c_parser_external_declaration(c_parser*)
(c-parser.c:1690)
==9759==by 0x91B25E: c_parser_translation_unit(c_parser*) (c-parser.c:1563)
==9759==by 0x9590A4: c_parse_file() (c-parser.c:21524)
==9759==by 0x9E308E: c_common_parse_file() (c-opts.c:1185)
If n2 != 0, newlen is computed as n1 + n2, but that doesn't take into
account for the , that is added in between the two.
The following patch ought to fix both the buffer overflow (by adding 1 if n2
is non-zero), memory leak (freeing newspec buffer after creating the string;
I've considered using XALLOCAVEC instead, but I believe the string can be
arbitrarily long on functions with thousands of arguments), using XNEWVEC
instead of (type *) xmalloc, using auto_diagnostic_group to bind warning +
inform together and fixes a typo in the documentation.
Ok for trunk if it passes bootstrap/regtest on x86_64-linux and i686-linux?
Thanks for the fix.
The buffer overflow enhancement I posted a couple of weeks ago has
logic to detect very simple forms of this bug:
https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00812.html
It knows how to figure out that the strcpy call below overflows:
void* f1 (const char *s1)
{
__SIZE_TYPE__ n1 = __builtin_strlen (s1);
char *s2 = __builtin_malloc (n1);
__builtin_strcpy (s2, s1);
return s2;
}
warning: ‘__builtin_strcpy’ writing one too many bytes into a region
of a size that depends on ‘strlen’ [-Wstringop-overflow=]
5 | __builtin_strcpy (s2, s1);
| ^
Unfortunately, it doesn't yet know how to see the similar problem
in more complicated code such as this:
void* f2 (const char *s1, const char *s2)
{
__SIZE_TYPE__ n1 = __builtin_strlen (s1);
__SIZE_TYPE__ n2 = __builtin_strlen (s2);
char *s3 = __builtin_malloc (n1 + n2);
__builtin_strcpy (s3, s1);
__builtin_strcat (s3, s2);
return s3;
}
Detecting that will take quite a bit more work. Some sort of
a symbolic constraint evaluation engine.
Martin
2019-11-23 Jakub Jelinek
PR middle-end/83859
* doc/extend.texi (attribute access): Fix a typo.
* c-attribs.c (append_access_attrs): Avoid bu
Re: [PATCH v3] PR92398: Fix testcase failure of pr72804.c
Hi,
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr72804-1.c
>
>> +/* store generates difference instructions as below:
>> + P9: mtvsrdd;xxlnot;stxv.
>> + P8/P7/P6 LE: not;not;std;std.
>> + P8 BE: mtvsrd;mtvsrd;xxpermdi;xxlnor;stxvd2x.
>> + P7/P6 BE: std;std;addi;lxvd2x;xxlnor;stxvd2x. */
>
> But it should generate just the first or second. So just document that,
> and base the tests around that as well?
check_effective_target_p8 is needed as P7/P6 BE has two unexpected std?
Also I split the store to new head file and rename it to pr92398.h.
Others are updated as below patch, Thanks:
P9LE generated instruction is not worse than P8LE.
mtvsrdd;xxlnot;stxv vs. not;not;std;std.
Update the test case to fix failures.
gcc/testsuite/ChangeLog:
2019-11-25 Luo Xiong Hu
testsuite/pr92398
* gcc.target/powerpc/pr72804.c: Split the store function to...
* gcc.target/powerpc/pr92398.h: ... this one. New.
* gcc.target/powerpc/pr92398.p9+.c: New.
* gcc.target/powerpc/pr92398.p9-.c: New.
* lib/target-supports.exp (check_effective_target_p8): New.
(check_effective_target_p9+): New.
---
gcc/testsuite/gcc.target/powerpc/pr72804.c| 19 --
gcc/testsuite/gcc.target/powerpc/pr92398.h| 17
.../gcc.target/powerpc/pr92398.p9+.c | 10 ++
.../gcc.target/powerpc/pr92398.p9-.c | 10 ++
gcc/testsuite/lib/target-supports.exp | 20 +++
5 files changed, 61 insertions(+), 15 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr92398.h
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
diff --git a/gcc/testsuite/gcc.target/powerpc/pr72804.c
b/gcc/testsuite/gcc.target/powerpc/pr72804.c
index 10e37caed6b..0b083a44ede 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr72804.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr72804.c
@@ -1,7 +1,6 @@
/* { dg-do compile { target { lp64 } } } */
-/* { dg-skip-if "" { powerpc*-*-darwin* } } */
/* { dg-require-effective-target powerpc_vsx_ok } */
-/* { dg-options "-O2 -mvsx -fno-inline-functions --param
max-inline-insns-single-O2=200" } */
+/* { dg-options "-O2 -mvsx" } */
__int128_t
foo (__int128_t *src)
@@ -9,17 +8,7 @@ foo (__int128_t *src)
return ~*src;
}
-void
-bar (__int128_t *dst, __int128_t src)
-{
- *dst = ~src;
-}
-/* { dg-final { scan-assembler-times "not " 4 } } */
-/* { dg-final { scan-assembler-times "std " 2 } } */
-/* { dg-final { scan-assembler-times "ld " 2 } } */
-/* { dg-final { scan-assembler-not "lxvd2x" } } */
-/* { dg-final { scan-assembler-not "stxvd2x" } } */
-/* { dg-final { scan-assembler-not "xxpermdi" } } */
-/* { dg-final { scan-assembler-not "mfvsrd" } } */
-/* { dg-final { scan-assembler-not "mfvsrd" } } */
+/* { dg-final { scan-assembler-times {\mld\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mnot\M} 2 } } */
+/* { dg-final { scan-assembler-not {\mlxvd2x\M} } }*/
diff --git a/gcc/testsuite/gcc.target/powerpc/pr92398.h
b/gcc/testsuite/gcc.target/powerpc/pr92398.h
new file mode 100644
index 000..184d02d3521
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr92398.h
@@ -0,0 +1,17 @@
+/* This test code is included into pr92398.p9-.c and pr92398.p9+.c.
+ The two files have the tests for the number of instructions generated for
+ P9- versus P9+.
+
+ store generates difference instructions as below:
+ P9+: mtvsrdd;xxlnot;stxv.
+ P8/P7/P6 LE: not;not;std;std.
+ P8 BE: mtvsrd;mtvsrd;xxpermdi;xxlnor;stxvd2x.
+ P7/P6 BE: std;std;addi;lxvd2x;xxlnor;stxvd2x.
+ P9+ and P9- LE are expected, P6/P7/P8 BE are unexpected. */
+
+void
+bar (__int128_t *dst, __int128_t src)
+{
+ *dst = ~src;
+}
+
diff --git a/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
b/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
new file mode 100644
index 000..2ebe2025cef
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { lp64 && p9+ } } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-O2 -mvsx" } */
+
+/* { dg-final { scan-assembler-times {\mmtvsrdd\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mxxlnor\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mstxv\M} 1 } } */
+
+/* Source code for the test in pr92398.h */
+#include "pr92398.h"
diff --git a/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
b/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
new file mode 100644
index 000..eaf0ddb86eb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { lp64 && {! p9+} } } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-O2 -mvsx" } */
+
+/* { dg-final { scan-assembler-times {\mnot\M} 2 { xfail be } } } */
+/* { dg-final { scan-assembler-times {\mstd\M} 2 { xfail { {p8} && {be} } } }
} */
+
+/* Sourc
Re: [patch, committed] PR92100 Formatted stream IO irreproducible read with binary data in file
On 11/24/19 2:20 PM, Jerry DeLisle wrote: Committed this as simple one liner. I will probably backport this in a few days. Jerry Committed r278660 M libgfortran/ChangeLog M libgfortran/io/transfer.c 2019-11-24 Jerry DeLisle PR fortran/92100 io/transfer.c (data_transfer_init_worker): Use fbuf_reset instead of fbuf_flush before the seek. Note that fbuf_reset calls fbuf_flush and adjusts fbuf pointers. Also commited a testcase, streamio_18.f90. Jerry
Re: [PATCH, rs6000] Refactor FP vector comparison operators
Hi Segher,
on 2019/11/23 上午12:08, Segher Boessenkool wrote:
> Hi!
>> 2019-11-21 Kewen Lin
>>
>> * config/rs6000/vector.md (vector_fp_comparison_simple):
>> New code iterator.
>> (vector_fp_comparison_complex): Likewise.
>> (vector_ for VEC_F and
>> vector_fp_comparison_simple): New define_and_split.
>
> (You don't have to wrap so early... Line length is 80 columns.)
>
>> +;; code iterators and attributes for vector FP comparison operators:
>> +(define_code_iterator
>> +vector_fp_comparison_simple [lt le ne ungt unge unlt unle])
>> +(define_code_iterator
>> +vector_fp_comparison_complex [ltgt uneq unordered ordered])
>
> Please indent by two spaces, not a tab.
>
>> +; For lt/le/ne/ungt/unge/unlt/unle:
>> +; lt(a,b) = gt(b,a)
>> +; le(a,b) = ge(b,a)
>> +; unge(a,b) = ~lt(a,b)
>> +; unle(a,b) = ~gt(a,b)
>> +; ne(a,b) = ~eq(a,b)
>> +; ungt(a,b) = ~le(a,b)
>> +; unlt(a,b) = ~ge(a,b)
>> +(define_insn_and_split "vector_"
>>[(set (match_operand:VEC_F 0 "vfloat_operand")
>> +(vector_fp_comparison_simple:VEC_F
>> + (match_operand:VEC_F 1 "vfloat_operand")
>> + (match_operand:VEC_F 2 "vfloat_operand")))]
>
> Indent is weird here as well.
>
>> +; For ltgt/uneq/ordered/unordered:
>> +; ltgt: gt(a,b) | gt(b,a)
>> +; uneq: ~(gt(a,b) | gt(b,a))
>> +; ordered: ge(a,b) | ge(b,a)
>> +; unordered: ~(ge(a,b) | ge(b,a))
>> +(define_insn_and_split "vector_"
>>[(set (match_operand:VEC_F 0 "vfloat_operand")
>> +(vector_fp_comparison_complex:VEC_F
>> + (match_operand:VEC_F 1 "vfloat_operand")
>> + (match_operand:VEC_F 2 "vfloat_operand")))]
>> + "VECTOR_UNIT_ALTIVEC_OR_VSX_P (mode) && can_create_pseudo_p ()"
>>"#"
>> + "&& can_create_pseudo_p ()"
>
> So hrm, if we do that here we can as well do that in the previous
> splitter as well (and not do the operands[0] thing) (sorry for going
> back on this -- I have said that before haven't I?)
>
>> + switch (cond)
>> +{
>> +case LTGT:
>> + cond = GT;
>> + break;
>> +case ORDERED:
>> + cond = GE;
>> + break;
>> +default:
>> + gcc_unreachable ();
>> +}
>
> It feels a bit lighter (and is shorter) if you write
>
> if (cond == LTGT)
> cond = GT;
> else if (cond == ORDERED)
> cond = GE;
> else
> gcc_unreachable ();
>
>
> Okay for trunk with those trivialities taken care of one way or the
> other. Thanks!
>
Updated it as all your suggestions above and committed in r278665.
Thanks again!
BR,
Kewen
Re: [Patch,Fortran] PR92629 - ICE in convert_mpz_to_unsigned, at fortran/simplify.c:173
On Sun, Nov 24, 2019 at 10:47 PM Harald Anlauf wrote: > > In convert_mpz_to_unsigned, an assert was triggered when a positive > argument larger than HUGE() of the respective kind was passed while > -fno-range-check was specified. The assert should be consistently > skipped in this case. > > Regtested on x86_64-pc-linux-gnu. > > OK for trunk / 9 / 8? > > Thanks, > Harald > > 2019-11-24 Harald Anlauf > > PR fortran/92629 > * simplify.c (convert_mpz_to_unsigned): Skip assert for argument > range when -fno-range-check is specified. > > > 2019-11-24 Harald Anlauf > > PR fortran/92629 > * gfortran.dg/pr92629.f90: New testcase. Ok, thanks. -- Janne Blomqvist
