Re: [PATCH] avoid calling alloca(0)
On 11/20/16 00:43, Martin Sebor wrote:
> As best I can tell the result isn't actually used (the code that
> uses the result gets branched over). GCC just doesn't see it.
> I verified this by changing the XALLOCAVEC macro to
>
> #define XALLOCAVEC(T, N) (N ? alloca (sizeof (T) * N) : 0)
>
> and bootstrapping and running the regression test suite with
> no apparent regressions.
>
I would like this solution with braces around N better than
allocating one element when actually zero were requested.
The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.
>> you should also include glibc and busybox, to be sure.
>
> Thanks for the suggestion. I've done that and found no instances
> of any of these warnings in either Busybox 1.25.1 or Glibc trunk.
>
Good. I see many alloca calls all over, but mostly in the runtime
libraries, that don't use -Werror.
Have you done a bootstrap with all languages?
Were there warnings in the target libraries (note they don't use
-Werror, so you have to grep)?
I am a bit worried about that suggestion in libiberty/alloca.c:
"It is a good idea to use alloca(0) in
your main control loop, etc. to force garbage collection."
Because:
#include
#include
int main()
{
void* p;
int i;
for (i=0 ; i<100 ; i++)
{
p = alloca(0);
printf("%p\n", p);
}
}
... is fine, and allocates no memory.
But if I change that to alloca(1) the stack will blow up.
Maybe it would be better to have a different warning option
for malloc/realloc with zero and for alloca with zero?
Bernd.
New French PO file for 'gcc' (version 6.2.0)
Hello, gentle maintainer. This is a message from the Translation Project robot. A revised PO file for textual domain 'gcc' has been submitted by the French team of translators. The file is available at: http://translationproject.org/latest/gcc/fr.po (This file, 'gcc-6.2.0.fr.po', has just now been sent to you in a separate email.) All other PO files for your package are available in: http://translationproject.org/latest/gcc/ Please consider including all of these in your next release, whether official or a pretest. Whenever you have a new distribution with a new version number ready, containing a newer POT file, please send the URL of that distribution tarball to the address below. The tarball may be just a pretest or a snapshot, it does not even have to compile. It is just used by the translators when they need some extra translation context. The following HTML page has been updated: http://translationproject.org/domain/gcc.html If any question arises, please contact the translation coordinator. Thank you for all your work, The Translation Project robot, in the name of your translation coordinator.
Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)
Hi,
when you add a returns_nonnull to the builtin alloca
then this code in tree-vrp.c (gimple_stmt_nonzero_warnv_p)
should go away:
if (flag_delete_null_pointer_checks &&
lookup_attribute ("returns_nonnull",
TYPE_ATTRIBUTES (gimple_call_fntype (stmt
return true;
return gimple_alloca_call_p (stmt);
Regarding __builtin_alloca_with_align, I see no test cases
for that function:
__builtin_alloca_with_align is a bit special, because it allocates
more than the first parameter says, I think it allocates (if size != 0)
size + align/8 in order to be able to return an aligned object.
What if align is very large?
I still would prefer separate -walloc-zero and -Walloca-zero
options. Likewise for the -Walloc-larger-than and -Walloca-larger-than
or better -Wmalloc-larger-than and -Walloca-larger-than ?
Bernd.
Re: [libgcc] Protect __TMC_END__ - __TMC_LIST__ == 0
On Fri, 4 Nov 2016, Andrew Pinski wrote: On Fri, Nov 4, 2016 at 7:08 AM, Marc Glisse wrote: Ping https://gcc.gnu.org/ml/gcc-patches/2016-10/msg02220.html I think this is obvious. Ok, I've committed it. -- Marc Glisse
PR78153
Hi,
As suggested by Martin in PR78153 strlen's return value cannot exceed
PTRDIFF_MAX.
So I set it's range to [0, PTRDIFF_MAX - 1] in extract_range_basic()
in the attached patch.
However it regressed strlenopt-3.c:
Consider fn1() from strlenopt-3.c:
__attribute__((noinline, noclone)) size_t
fn1 (char *p, char *q)
{
size_t s = strlen (q);
strcpy (p, q);
return s - strlen (p);
}
The optimized dump shows the following:
__attribute__((noclone, noinline))
fn1 (char * p, char * q)
{
size_t s;
size_t _7;
long unsigned int _9;
:
s_4 = strlen (q_3(D));
_9 = s_4 + 1;
__builtin_memcpy (p_5(D), q_3(D), _9);
_7 = 0;
return _7;
}
which introduces the regression, because the test expects "return 0;" in fn1().
The issue seems to be in vrp2:
Before the patch:
Visiting statement:
s_4 = strlen (q_3(D));
Found new range for s_4: VARYING
Visiting statement:
_1 = s_4;
Found new range for _1: [s_4, s_4]
marking stmt to be not simulated again
Visiting statement:
_7 = s_4 - _1;
Applying pattern match.pd:111, gimple-match.c:27997
Match-and-simplified s_4 - _1 to 0
Intersecting
[0, 0]
and
[0, +INF]
to
[0, 0]
Found new range for _7: [0, 0]
__attribute__((noclone, noinline))
fn1 (char * p, char * q)
{
size_t s;
long unsigned int _1;
long unsigned int _9;
:
s_4 = strlen (q_3(D));
_9 = s_4 + 1;
__builtin_memcpy (p_5(D), q_3(D), _9);
_1 = s_4;
return 0;
}
After the patch:
Visiting statement:
s_4 = strlen (q_3(D));
Intersecting
[0, 9223372036854775806]
and
[0, 9223372036854775806]
to
[0, 9223372036854775806]
Found new range for s_4: [0, 9223372036854775806]
marking stmt to be not simulated again
Visiting statement:
_1 = s_4;
Intersecting
[0, 9223372036854775806] EQUIVALENCES: { s_4 } (1 elements)
and
[0, 9223372036854775806]
to
[0, 9223372036854775806] EQUIVALENCES: { s_4 } (1 elements)
Found new range for _1: [0, 9223372036854775806]
marking stmt to be not simulated again
Visiting statement:
_7 = s_4 - _1;
Intersecting
~[9223372036854775807, 9223372036854775809]
and
~[9223372036854775807, 9223372036854775809]
to
~[9223372036854775807, 9223372036854775809]
Found new range for _7: ~[9223372036854775807, 9223372036854775809]
marking stmt to be not simulated again
__attribute__((noclone, noinline))
fn1 (char * p, char * q)
{
size_t s;
long unsigned int _1;
size_t _7;
long unsigned int _9;
:
s_4 = strlen (q_3(D));
_9 = s_4 + 1;
__builtin_memcpy (p_5(D), q_3(D), _9);
_1 = s_4;
_7 = s_4 - _1;
return _7;
}
Then forwprop4 turns
_1 = s_4
_7 = s_4 - _1
into
_7 = 0
and we end up with:
_7 = 0
return _7
in optimized dump.
Running ccp again after forwprop4 trivially solves the issue, however
I am not sure if we want to run ccp again ?
The issue is probably with extract_range_from_ssa_name():
For _1 = s_4
Before patch:
VR for s_4 is set to varying.
So VR for _1 is set to [s_4, s_4] by extract_range_from_ssa_name.
Since VR for _1 is [s_4, s_4] it implicitly implies that _1 is equal to s_4,
and vrp is able to transform _7 = s_4 - _1 to _7 = 0 (by using
match.pd pattern x - x -> 0).
After patch:
VR for s_4 is set to [0, PTRDIFF_MAX - 1]
And correspondingly VR for _1 is set to [0, PTRDIFF_MAX - 1]
so IIUC, we then lose the information that _1 is equal to s_4,
and vrp doesn't transform _7 = s_4 - _1 to _7 = 0.
forwprop4 does that because it sees that s_4 and _1 are equivalent.
Does this sound correct ?
I am not sure how to proceed with the patch,
and would be grateful for suggestions.
Thanks,
Prathamesh
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr78153-1.c
b/gcc/testsuite/gcc.dg/tree-ssa/pr78153-1.c
new file mode 100644
index 000..2530ba0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr78153-1.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-evrp-slim" } */
+
+void f(const char *s)
+{
+ if (__PTRDIFF_MAX__ <= __builtin_strlen (s))
+__builtin_abort ();
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_abort" "evrp" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr78153-2.c
b/gcc/testsuite/gcc.dg/tree-ssa/pr78153-2.c
new file mode 100644
index 000..de70450
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr78153-2.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-evrp-slim" } */
+
+void f(const char *s)
+{
+ __PTRDIFF_TYPE__ n = __builtin_strlen (s);
+ if (n < 0)
+__builtin_abort ();
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_abort" "evrp" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index c2a4133..d17b413 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -4013,6 +4013,16 @@ extract_range_basic (value_range *vr, gimple *stmt)
: vrp_val_max (type), NULL);
}
return;
+ case CFN_BUILT_IN_STRLEN:
+ {
+ tree type = TREE_TYPE (gimple_call_lhs (stmt));
+ unsigned HOST_WIDE_INT max =
+ TREE_INT_CST_LOW (vrp_val_max (ptrdiff_type_node)) - 1;
+
+
Re: PR78153
On Sun, Nov 20, 2016 at 07:20:20PM +0530, Prathamesh Kulkarni wrote:
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -4013,6 +4013,16 @@ extract_range_basic (value_range *vr, gimple *stmt)
> : vrp_val_max (type), NULL);
> }
> return;
> + case CFN_BUILT_IN_STRLEN:
> + {
> + tree type = TREE_TYPE (gimple_call_lhs (stmt));
> + unsigned HOST_WIDE_INT max =
> + TREE_INT_CST_LOW (vrp_val_max (ptrdiff_type_node)) - 1;
Wrong formatting, = should go on the next line, and should be indented only
2 columns more than the previous line. Plus TREE_INT_CST_LOW really
shouldn't be used in new code. You should use tree_to_uhwi or tree_to_shwi
instead. Why the -1? Can you just
fold_convert (type, TYPE_MAX_VALUE (ptrdiff_type_node)); ?
Or, if you really want the -1, e.g. wide_int max = vrp_val_max
(ptrdiff_type_node);
wide_int_to_tree (type, max - 1);
or something similar.
> +
> + set_value_range (vr, VR_RANGE, build_int_cst (type, 0),
> + build_int_cst (type, max), NULL);
> + }
> + return;
> default:
> break;
> }
Jakub
Re: PR78153
On 20 November 2016 at 19:34, Jakub Jelinek wrote:
> On Sun, Nov 20, 2016 at 07:20:20PM +0530, Prathamesh Kulkarni wrote:
>> --- a/gcc/tree-vrp.c
>> +++ b/gcc/tree-vrp.c
>> @@ -4013,6 +4013,16 @@ extract_range_basic (value_range *vr, gimple *stmt)
>> : vrp_val_max (type), NULL);
>> }
>> return;
>> + case CFN_BUILT_IN_STRLEN:
>> + {
>> + tree type = TREE_TYPE (gimple_call_lhs (stmt));
>> + unsigned HOST_WIDE_INT max =
>> + TREE_INT_CST_LOW (vrp_val_max (ptrdiff_type_node)) - 1;
>
> Wrong formatting, = should go on the next line, and should be indented only
> 2 columns more than the previous line. Plus TREE_INT_CST_LOW really
> shouldn't be used in new code. You should use tree_to_uhwi or tree_to_shwi
> instead. Why the -1? Can you just
> fold_convert (type, TYPE_MAX_VALUE (ptrdiff_type_node)); ?
> Or, if you really want the -1, e.g. wide_int max = vrp_val_max
> (ptrdiff_type_node);
> wide_int_to_tree (type, max - 1);
> or something similar.
Hi Jakub,
Thanks for the suggestions.
Sorry I wrote misleading info in the patch. As per PR, strlen's return value
should always be less than PTRDIFF_MAX, so I am setting the range to
[0, PTRDIFF_MAX - 1].
I will use wide_int in the next version of patch.
Thanks,
Prathamesh
>> +
>> + set_value_range (vr, VR_RANGE, build_int_cst (type, 0),
>> + build_int_cst (type, max), NULL);
>> + }
>> + return;
>> default:
>> break;
>> }
>
> Jakub
Re: [PATCH, Fortran, pr78395, v1] [OOP] error on polymorphic assignment
Hi Janus, thanks for the review. Committed to trunk as r242637. Will wait one week before committing to 6. Regards, Andre On Sat, 19 Nov 2016 16:14:54 +0100 Janus Weil wrote: > Hi Andre, > > > When checking the shortened example in comment #3 one gets a segfault, > > because v6 is not allocated explicitly. The initial example made sure, that > > v6 was allocated. > > sorry, I guess that's my fault. I blindly removed the allocate > statement when looking for a reduced test case for the compile-time > error. > > > > Btw, when using the in gcc-7 available > > polymorphic assign, then v6 is actually auto-allocated and the program runs > > fine. So what are your opinions on the auto-allocation issue? > > I suspect that auto-allocation does not apply to defined assignment, > but I'm not fully sure. Looking in the F08 standard, it seems to be > mentioned in 7.2.1.3, but not in 7.2.1.4. > > As Thomas mentioned, you could take that question to c.l.f. to get a > more qualified answer and/or open a follow-up PR for it. > > > > This patch fixes the wrong error messages in both gcc-7 and gcc-6. > > Bootstraped and regtested on x86_64-linux/F23 for gcc-7 and -6. Ok for trunk > > and gcc-6? > > Yes, looks good to me (at least for trunk; gcc-6 if you like). > > Thanks for the patch, > Janus -- Andre Vehreschild * Email: vehre ad gmx dot de
Re: [PATCH, Fortran, pr78395, v1] [OOP] error on polymorphic assignment
Whoops, hit send to fast. Here's the patch committed.
- Andre
On Sun, 20 Nov 2016 15:23:16 +0100
Andre Vehreschild wrote:
> Hi Janus,
>
> thanks for the review. Committed to trunk as r242637. Will wait one week
> before committing to 6.
>
> Regards,
> Andre
>
> On Sat, 19 Nov 2016 16:14:54 +0100
> Janus Weil wrote:
>
> > Hi Andre,
> >
> > > When checking the shortened example in comment #3 one gets a segfault,
> > > because v6 is not allocated explicitly. The initial example made sure,
> > > that v6 was allocated.
> >
> > sorry, I guess that's my fault. I blindly removed the allocate
> > statement when looking for a reduced test case for the compile-time
> > error.
> >
> >
> > > Btw, when using the in gcc-7 available
> > > polymorphic assign, then v6 is actually auto-allocated and the program
> > > runs fine. So what are your opinions on the auto-allocation issue?
> >
> > I suspect that auto-allocation does not apply to defined assignment,
> > but I'm not fully sure. Looking in the F08 standard, it seems to be
> > mentioned in 7.2.1.3, but not in 7.2.1.4.
> >
> > As Thomas mentioned, you could take that question to c.l.f. to get a
> > more qualified answer and/or open a follow-up PR for it.
> >
> >
> > > This patch fixes the wrong error messages in both gcc-7 and gcc-6.
> > > Bootstraped and regtested on x86_64-linux/F23 for gcc-7 and -6. Ok for
> > > trunk and gcc-6?
> >
> > Yes, looks good to me (at least for trunk; gcc-6 if you like).
> >
> > Thanks for the patch,
> > Janus
>
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
Index: gcc/fortran/ChangeLog
===
--- gcc/fortran/ChangeLog (Revision 242636)
+++ gcc/fortran/ChangeLog (Arbeitskopie)
@@ -1,3 +1,9 @@
+2016-11-20 Andre Vehreschild
+
+ PR fortran/78395
+ * resolve.c (resolve_typebound_function): Prevent stripping of refs,
+ when the base-expression is a class' typed one.
+
2016-11-18 Richard Sandiford
Alan Hayward
David Sherwood
Index: gcc/fortran/resolve.c
===
--- gcc/fortran/resolve.c (Revision 242636)
+++ gcc/fortran/resolve.c (Arbeitskopie)
@@ -6140,7 +6140,7 @@
gfc_free_ref_list (class_ref->next);
class_ref->next = NULL;
}
- else if (e->ref && !class_ref)
+ else if (e->ref && !class_ref && expr->ts.type != BT_CLASS)
{
gfc_free_ref_list (e->ref);
e->ref = NULL;
Index: gcc/testsuite/ChangeLog
===
--- gcc/testsuite/ChangeLog (Revision 242636)
+++ gcc/testsuite/ChangeLog (Arbeitskopie)
@@ -1,3 +1,8 @@
+2016-11-20 Andre Vehreschild
+
+ PR fortran/78395
+ * gfortran.dg/typebound_operator_21.f03: New test.
+
2016-11-20 Marc Glisse
* gcc.dg/tree-ssa/divide-5.c: New file.
Index: gcc/testsuite/gfortran.dg/typebound_operator_21.f03
===
--- gcc/testsuite/gfortran.dg/typebound_operator_21.f03 (nicht existent)
+++ gcc/testsuite/gfortran.dg/typebound_operator_21.f03 (Arbeitskopie)
@@ -0,0 +1,78 @@
+! { dg-do run }
+!
+! Test that pr78395 is fixed.
+! Contributed by Chris MacMackin and Janus Weil
+
+module types_mod
+ implicit none
+
+ type, public :: t1
+integer :: a
+ contains
+procedure :: get_t2
+ end type
+
+ type, public :: t2
+integer :: b
+ contains
+procedure, pass(rhs) :: mul2
+procedure :: assign
+generic :: operator(*) => mul2
+generic :: assignment(=) => assign
+ end type
+
+contains
+
+ function get_t2(this)
+class(t1), intent(in) :: this
+class(t2), allocatable :: get_t2
+type(t2), allocatable :: local
+allocate(local)
+local%b = this%a
+call move_alloc(local, get_t2)
+ end function
+
+ function mul2(lhs, rhs)
+class(t2), intent(in) :: rhs
+integer, intent(in) :: lhs
+class(t2), allocatable :: mul2
+type(t2), allocatable :: local
+allocate(local)
+local%b = rhs%b*lhs
+call move_alloc(local, mul2)
+ end function
+
+ subroutine assign(this, rhs)
+class(t2), intent(out) :: this
+class(t2), intent(in) :: rhs
+select type(rhs)
+type is(t2)
+ this%b = rhs%b
+class default
+ error stop
+end select
+ end subroutine
+
+end module
+
+
+program minimal
+ use types_mod
+ implicit none
+
+ class(t1), allocatable :: v4
+ class(t2), allocatable :: v6
+
+ allocate(v4, source=t1(4))
+ allocate(v6)
+ v6 = 3 * v4%get_t2()
+
+ select type (v6)
+type is (t2)
+ if (v6%b /= 12) error stop
+class default
+ error stop
+ end select
+ deallocate(v4, v6)
+end
+
Re: PR61409: -Wmaybe-uninitialized false-positive with -O2
On 11/16/2016 03:57 PM, Jeff Law wrote:
On 11/02/2016 11:16 AM, Aldy Hernandez wrote:
Hi Jeff.
As discussed in the PR, here is a patch exploring your idea of ignoring
unguarded uses if we can prove that the guards for such uses are
invalidated by the uninitialized operand paths being executed.
This is an updated patch from my suggestion in the PR. It bootstraps
with no regression on x86-64 Linux, and fixes the PR in question.
As the "NOTE:" in the code states, we could be much smarter when
invalidating predicates, but for now let's do straight negation which
works for the simple case. We could expand on this in the future.
OK for trunk?
curr
commit 8375d7e28c1a798dd0cc0f487d7fa1068d9eb124
Author: Aldy Hernandez
Date: Thu Aug 25 10:44:29 2016 -0400
PR middle-end/61409
* tree-ssa-uninit.c (use_pred_not_overlap_with_undef_path_pred):
Remove reference to missing NUM_PREDS in function comment.
(can_one_predicate_be_invalidated_p): New.
(can_chain_union_be_invalidated_p): New.
(flatten_out_predicate_chains): New.
(uninit_ops_invalidate_phi_use): New.
(is_use_properly_guarded): Call uninit_ops_invalidate_phi_use.
[ snip ]
+static bool
+can_one_predicate_be_invalidated_p (pred_info predicate,
+vec worklist)
+{
+ for (size_t i = 0; i < worklist.length (); ++i)
+{
+ pred_info *p = worklist[i];
+
+ /* NOTE: This is a very simple check, and only understands an
+ exact opposite. So, [i == 0] is currently only invalidated
+ by [.NOT. i == 0] or [i != 0]. Ideally we should also
+ invalidate with say [i > 5] or [i == 8]. There is certainly
+ room for improvement here. */
+ if (pred_neg_p (predicate, *p))
It's good enough for now. I saw some other routines that might allow us
to handle more cases. I'm OK with faulting those in if/when we see such
cases in real code.
Ok.
+
+/* Return TRUE if executing the path to some uninitialized operands in
+ a PHI will invalidate the use of the PHI result later on.
+
+ UNINIT_OPNDS is a bit vector specifying which PHI arguments have
+ arguments which are considered uninitialized.
+
+ USE_PREDS is the pred_chain_union specifying the guard conditions
+ for the use of the PHI result.
+
+ What we want to do is disprove each of the guards in the factors of
+ the USE_PREDS. So if we have:
+
+ # USE_PREDS guards of:
+ #1. i > 5 && i < 100
+ #2. j > 10 && j < 88
Are USE_PREDS joined by an AND or IOR? I guess based on their type it
must be IOR. Thus to get to a use #1 or #2 must be true. So to prove
we can't reach a use, we have to prove that #1 and #2 are both not true.
Right?
IOR, so yes.
+
+static bool
+uninit_ops_invalidate_phi_use (gphi *phi, unsigned uninit_opnds,
+ pred_chain_union use_preds)
+{
+ /* Look for the control dependencies of all the uninitialized
+ operands and build predicates describing them. */
+ unsigned i;
+ pred_chain_union uninit_preds[32];
+ memset (uninit_preds, 0, sizeof (pred_chain_union) * 32);
+ for (i = 0; i < MIN (32, gimple_phi_num_args (phi)); i++)
Can you replace the magic "32" with a file scoped const or #define? I
believe there's 2 existing uses of a magic "32" elsewhere in
tree-ssa-uninit.c as well.
Done.
+
+ /* Build the control dependency chain for `i'... */
+ if (compute_control_dep_chain (find_dom (e->src),
+ e->src,
+ dep_chains,
+ &num_chains,
+ &cur_chain,
+ &num_calls))
Does this miss the control dependency carried by E itself.
ie, if e->src ends in a conditional, shouldn't that conditional be
reflected in the control dependency chain as well? I guess we'd have to
have the notion of computing the control dependency for an edge rather
than a block. It doesn't look like compute_control_dep_chain is ready
for that. I'm willing to put that into a "future work" bucket.
Hmmm, probably not. So yeah, let's put that in the future work bucket :).
So I think just confirming my question about how USE_PREDS are joined at
the call to uninit_opts_invalidate_phi_use and fixing the magic 32 to be
a file scoped const or a #define and this is good to go on the trunk.
I've retested with no regressions on x86-64 Linux.
OK for trunk?
commit 42f32d1e064613d031c1e82f259d89f290355dd2
Author: Aldy Hernandez
Date: Thu Aug 25 10:44:29 2016 -0400
PR middle-end/61409
* tree-ssa-uninit.c: Define new global max_phi_args.
(compute_uninit_opnds_pos): Use max_phi_args.
(prune_uninit_phi_opnds): Same.
(use_pred_not_overlap_with_undef_path_pred): Remove reference to
missing NUM_PREDS in function comment.
(can_one_predicate_be_invalidated_p): New.
(can_chain_union_be_invalidated_p): New.
(flatten_out_predicate_chains): New.
(uninit_ops_invalidate_ph
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On Nov 19, 2016, at 1:59 PM, Andrew Burgess wrote:
>> So, your new test fails on arm* targets:
>
> After a little digging I think the problem might be that
> -freorder-blocks-and-partition is not supported on arm.
>
> This should be detected as the new tests include:
>
>/* { dg-require-effective-target freorder } */
>
> however this test passed on arm as -freorder-blocks-and-partition does
> not issue any warning unless -fprofile-use is also passed.
>
> The patch below extends check_effective_target_freorder to check using
> -fprofile-use. With this change in place the tests are skipped on
> arm.
> All feedback welcome,
Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person
thinks this is the wrong solution.
Re: Default associative containers constructors/destructor/assignment
On 17/11/2016 18:52, Jonathan Wakely wrote:
On 28/10/16 21:42 +0200, François Dumont wrote:
+ template
+struct _Rb_tree_key_compare
+{
+ _Key_compare_M_key_compare;
+
+ _Rb_tree_key_compare()
+ _GLIBCXX_NOEXCEPT_IF(
+ is_nothrow_default_constructible<_Key_compare>::value)
+ : _M_key_compare()
+ { }
+
+ _Rb_tree_key_compare(const _Key_compare& __comp)
+ : _M_key_compare(__comp)
+ { }
+
+#if __cplusplus >= 201103L
+ _Rb_tree_key_compare(_Rb_tree_key_compare&& __x)
+ noexcept(is_nothrow_copy_constructible<_Key_compare>::value)
+ : _M_key_compare(__x._M_key_compare)
+ { }
+#endif
This constructor makes the type move-only (i.e. non-copyable) in
C++11 and later. It's copyable in C++98. Is that what you want?
I simply consider it was not a problem as, as you noticed, it is not used.
Adding defaulted copy operations would fix that. Even if we don't
actually need those copy operations, I'm uncomfortable with it being
copyable in C++98 and non-copyable otherwise.
Ok, I'll add a default copy constructor for consistency like this:
// Copy constructor added for consistency with C++98 mode.
_Rb_tree_key_compare(const _Rb_tree_compare&) = default;
+void
+_M_reset()
+{
+ _M_initialize();
+ _M_node_count = 0;
+}
This introduces a small change in behaviour, because _M_reset() now
does _M_header._M_color = _S_red, which it didn't do before. That
store is redundant. It could be avoided by just doing the three
assignments in _M_reset() instead of calling _M_initialize().
I thought it was ok to do this additional operation for the sake of
reusing _M_initialize(). I was compenciating it by avoiding default
initialization when there is something to move.
And we could remove _M_initialize() entirely, and remove the redundant
mem-initializers for _M_node_count (because it's set my _M_reset and
_M_move_data anyway):
_Rb_tree_header() _GLIBCXX_NOEXCEPT
{
_M_reset();
_M_header._M_color = _S_red;
}
#if __cplusplus >= 201103L
_Rb_tree_header(_Rb_tree_header&& __x) noexcept
{
if (__x._M_header._M_parent != nullptr)
_M_move_data(__x);
else
{
_M_reset();
_M_header._M_color = _S_red;
}
}
void
_M_move_data(_Rb_tree_header& __x)
{
_M_header._M_parent = __x._M_header._M_parent;
_M_header._M_left = __x._M_header._M_left;
_M_header._M_right = __x._M_header._M_right;
_M_header._M_parent->_M_parent = &_M_header;
_M_node_count = __x._M_node_count;
__x._M_reset();
}
#endif
void
_M_reset()
{
_M_header._M_parent = 0;
_M_header._M_left = &_M_header;
_M_header._M_right = &_M_header;
_M_node_count = 0;
}
Yes, looks nice, adopted.
Talking about _M_color I just realize that move semantic doesn't copy
_M_color. Shouldn't we capture it along with all the other _M_header
information ?
@@ -599,50 +674,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// Unused _Is_pod_comparator is kept as it is part of mangled
name.
template
-struct _Rb_tree_impl : public _Node_allocator
+struct _Rb_tree_impl
+: public _Node_allocator
+, public _Rb_tree_key_compare<_Key_compare>
+, public _Rb_tree_header
Do these need to be base classes, rather than data members?
We derive from the allocator to benefit from the empty base-class
optimization, but that isn't relevant for the _Rb_tree_key_compare and
_Rb_tree_header types. It *could* be relevant for the comparison
function, but would be an ABI change. We could do that ABI change
conditionally, for gnu-versioned-namespace builds, but that's still
possible using data members (e.g. have a data member that derives from
the comparison function and contains the header node and/or node count
members).
Making them data members would simply mean restoring the function
_Rb_tree_impl::_M_reset() and making it call reset on the member:
void
_M_reset() { _M_header._M_reset(); }
The minor convenience of inheriting this function from a base class
doesn't seem worth the stronger coupling that comes from using
inheritance.
Am I missing some other reason that inheritance is used here?
The purpose of this patch is to rely more on compiler especially in
regards to the noexcept qualifications. This is why I started isolating
each ressource in its own class in charge of its specificities. And I
leave to the compiler the work of combining those types. However I also
wanted to limit the impact of this patch on the _Rb_tree and still be
able to use things like this->_M_impl._M_node_count or
this->_M_impl._M_header. So the usage of inheritance.
- _Rb_tree_impl(const _Key_compare& __comp, const
_Node_allocator& __a)
- : _Node_allocator(__a), _M_key_compare(__comp), _M_header(),
-_M_node_count(0)
- { _M_initialize(); }
Please mention the removal of this constructor in the ch
Re: [PATCH] Enable Intel AVX512_4FMAPS and AVX512_4VNNIW instructions
On Sat, Nov 19, 2016 at 7:52 PM, Uros Bizjak wrote: > On Sat, Nov 19, 2016 at 6:24 PM, Jakub Jelinek wrote: >> On Sat, Nov 19, 2016 at 12:28:22PM +0100, Jakub Jelinek wrote: >>> On x86_64-linux with the 3 patches I'm not seeing any new FAILs >>> compared to before r242569, on i686-linux there is still: >>> +FAIL: gcc.target/i386/pr57756.c (test for errors, line 6) >>> +FAIL: gcc.target/i386/pr57756.c (test for warnings, line 14) >>> compared to pre-r242569 (so some further fix is needed). >> >> And finally here is yet another patch that fixes pr57756 on i686-linux. >> Ok for trunk together with the other 3 patches? > > OK for the whole patch series. Hm, I still see (both, 32bit and 64bit targets): In file included from /ssd/uros/gcc-build/gcc/include/immintrin.h:45:0,^M from /home/uros/gcc-svn/trunk/gcc/testsuite/gcc.target/i386/sse-22.c:223,^M from /home/uros/gcc-svn/trunk/gcc/testsuite/gcc.target/i386/sse-22a.c:7:^M /ssd/uros/gcc-build/gcc/include/avx5124fmapsintrin.h: In function '_mm512_maskz_4fmadd_ps':^M /ssd/uros/gcc-build/gcc/include/avx512fintrin.h:244:1: error: inlining failed in call to always_inline '_mm512_setzero_ps': target specific option mismatch^M In file included from /ssd/uros/gcc-build/gcc/include/immintrin.h:71:0,^M from /home/uros/gcc-svn/trunk/gcc/testsuite/gcc.target/i386/sse-22.c:223,^M from /home/uros/gcc-svn/trunk/gcc/testsuite/gcc.target/i386/sse-22a.c:7:^M /ssd/uros/gcc-build/gcc/include/avx5124fmapsintrin.h:77:17: note: called from here^M compiler exited with status 1 FAIL: gcc.target/i386/sse-22a.c (test for excess errors) Excess errors: /ssd/uros/gcc-build/gcc/include/avx512fintrin.h:244:1: error: inlining failed in call to always_inline '_mm512_setzero_ps': target specific option mismatch Uros.
Re: PR61409: -Wmaybe-uninitialized false-positive with -O2
On 11/20/2016 09:36 AM, Aldy Hernandez wrote:
On 11/16/2016 03:57 PM, Jeff Law wrote:
On 11/02/2016 11:16 AM, Aldy Hernandez wrote:
Hi Jeff.
As discussed in the PR, here is a patch exploring your idea of ignoring
unguarded uses if we can prove that the guards for such uses are
invalidated by the uninitialized operand paths being executed.
This is an updated patch from my suggestion in the PR. It bootstraps
with no regression on x86-64 Linux, and fixes the PR in question.
As the "NOTE:" in the code states, we could be much smarter when
invalidating predicates, but for now let's do straight negation which
works for the simple case. We could expand on this in the future.
OK for trunk?
curr
commit 8375d7e28c1a798dd0cc0f487d7fa1068d9eb124
Author: Aldy Hernandez
Date: Thu Aug 25 10:44:29 2016 -0400
PR middle-end/61409
* tree-ssa-uninit.c (use_pred_not_overlap_with_undef_path_pred):
Remove reference to missing NUM_PREDS in function comment.
(can_one_predicate_be_invalidated_p): New.
(can_chain_union_be_invalidated_p): New.
(flatten_out_predicate_chains): New.
(uninit_ops_invalidate_phi_use): New.
(is_use_properly_guarded): Call uninit_ops_invalidate_phi_use.
[ snip ]
+static bool
+can_one_predicate_be_invalidated_p (pred_info predicate,
+vec worklist)
+{
+ for (size_t i = 0; i < worklist.length (); ++i)
+{
+ pred_info *p = worklist[i];
+
+ /* NOTE: This is a very simple check, and only understands an
+ exact opposite. So, [i == 0] is currently only invalidated
+ by [.NOT. i == 0] or [i != 0]. Ideally we should also
+ invalidate with say [i > 5] or [i == 8]. There is certainly
+ room for improvement here. */
+ if (pred_neg_p (predicate, *p))
It's good enough for now. I saw some other routines that might allow us
to handle more cases. I'm OK with faulting those in if/when we see such
cases in real code.
Ok.
+
+/* Return TRUE if executing the path to some uninitialized operands in
+ a PHI will invalidate the use of the PHI result later on.
+
+ UNINIT_OPNDS is a bit vector specifying which PHI arguments have
+ arguments which are considered uninitialized.
+
+ USE_PREDS is the pred_chain_union specifying the guard conditions
+ for the use of the PHI result.
+
+ What we want to do is disprove each of the guards in the factors of
+ the USE_PREDS. So if we have:
+
+ # USE_PREDS guards of:
+ #1. i > 5 && i < 100
+ #2. j > 10 && j < 88
Are USE_PREDS joined by an AND or IOR? I guess based on their type it
must be IOR. Thus to get to a use #1 or #2 must be true. So to prove
we can't reach a use, we have to prove that #1 and #2 are both not true.
Right?
IOR, so yes.
+
+static bool
+uninit_ops_invalidate_phi_use (gphi *phi, unsigned uninit_opnds,
+ pred_chain_union use_preds)
+{
+ /* Look for the control dependencies of all the uninitialized
+ operands and build predicates describing them. */
+ unsigned i;
+ pred_chain_union uninit_preds[32];
+ memset (uninit_preds, 0, sizeof (pred_chain_union) * 32);
+ for (i = 0; i < MIN (32, gimple_phi_num_args (phi)); i++)
Can you replace the magic "32" with a file scoped const or #define? I
believe there's 2 existing uses of a magic "32" elsewhere in
tree-ssa-uninit.c as well.
Done.
+
+ /* Build the control dependency chain for `i'... */
+ if (compute_control_dep_chain (find_dom (e->src),
+ e->src,
+ dep_chains,
+ &num_chains,
+ &cur_chain,
+ &num_calls))
Does this miss the control dependency carried by E itself.
ie, if e->src ends in a conditional, shouldn't that conditional be
reflected in the control dependency chain as well? I guess we'd have to
have the notion of computing the control dependency for an edge rather
than a block. It doesn't look like compute_control_dep_chain is ready
for that. I'm willing to put that into a "future work" bucket.
Hmmm, probably not. So yeah, let's put that in the future work bucket :).
So I think just confirming my question about how USE_PREDS are joined at
the call to uninit_opts_invalidate_phi_use and fixing the magic 32 to be
a file scoped const or a #define and this is good to go on the trunk.
I've retested with no regressions on x86-64 Linux.
OK for trunk?
Yes. Thanks for taking my vague idea and making it real :-)
I've got another if you're interested...
jeff
Re: PR69741: Bad error in formal with array scalar loop counters
On Sat, Nov 19, 2016 at 03:44:12PM +0100, Harald Anlauf wrote:
> >
> > 2016-11-19 Harald Anlauf
> >
> > PR fortran/69741
> > * resolve.c (gfc_resolve_forall): Check for non-scalar index
> > variables.
> >
> > 2016-11-19 Harald Anlauf
> >
> > PR fortran/69741
> > * gfortran.dg/forall_18.f90: New testcase.
> >
Here's the patch that I committed.
Index: gcc/fortran/resolve.c
===
--- gcc/fortran/resolve.c (revision 242624)
+++ gcc/fortran/resolve.c (working copy)
@@ -9647,16 +9647,15 @@ gfc_resolve_forall (gfc_code *code, gfc_
static gfc_expr **var_expr;
static int total_var = 0;
static int nvar = 0;
- int old_nvar, tmp;
+ int i, old_nvar, tmp;
gfc_forall_iterator *fa;
- int i;
old_nvar = nvar;
/* Start to resolve a FORALL construct */
if (forall_save == 0)
{
- /* Count the total number of FORALL index in the nested FORALL
+ /* Count the total number of FORALL indices in the nested FORALL
construct in order to allocate the VAR_EXPR with proper size. */
total_var = gfc_count_forall_iterators (code);
@@ -9664,19 +9663,25 @@ gfc_resolve_forall (gfc_code *code, gfc_
var_expr = XCNEWVEC (gfc_expr *, total_var);
}
- /* The information about FORALL iterator, including FORALL index start, end
- and stride. The FORALL index can not appear in start, end or stride. */
+ /* The information about FORALL iterator, including FORALL indices start, end
+ and stride. An outer FORALL indice cannot appear in start, end or
stride. */
for (fa = code->ext.forall_iterator; fa; fa = fa->next)
{
+ /* Fortran 20008: C738 (R753). */
+ if (fa->var->ref && fa->var->ref->type == REF_ARRAY)
+ {
+ gfc_error ("FORALL index-name at %L must be a scalar variable "
+"of type integer", &fa->var->where);
+ continue;
+ }
+
/* Check if any outer FORALL index name is the same as the current
one. */
for (i = 0; i < nvar; i++)
{
if (fa->var->symtree->n.sym == var_expr[i]->symtree->n.sym)
- {
- gfc_error ("An outer FORALL construct already has an index "
-"with this name %L", &fa->var->where);
- }
+ gfc_error ("An outer FORALL construct already has an index "
+ "with this name %L", &fa->var->where);
}
/* Record the current FORALL index. */
Index: gcc/testsuite/gfortran.dg/forall_18.f90
===
--- gcc/testsuite/gfortran.dg/forall_18.f90 (nonexistent)
+++ gcc/testsuite/gfortran.dg/forall_18.f90 (working copy)
@@ -0,0 +1,16 @@
+! { dg-do compile }
+! PR fortran/69741 - improve error message for nonscalar FORALL index variables
+!
+subroutine check
+ integer :: ii(2), i
+ real :: a(3,2)
+
+ forall (ii(1)=1:3, i=1:2) ! { dg-error "scalar variable of type integer" }
+ a(ii(1),i) = ii(1) * i
+ end forall
+
+ forall (j=1:3, ii(2)=1:2) ! { dg-error "scalar variable of type integer" }
+ a(j,ii(2)) = j * ii(2)
+ end forall
+
+end subroutine check
--
Steve
Re: [PATCH] Delete GCJ
On 10.10.2016 09:58, Iain Sandoe wrote:
>
>> On 10 Oct 2016, at 05:03, Matthias Klose wrote:
>>
>> On 07.10.2016 10:30, Iain Sandoe wrote:
>>>
On 7 Oct 2016, at 00:58, Matthias Klose wrote:
On 06.10.2016 20:00, Mike Stump wrote:
> On Oct 6, 2016, at 9:56 AM, Rainer Orth
> wrote:
>> I wouldn't hard-fail, but completely disable objc-gc with an appropriate
>> warning. The Objective-C maintainers may have other preferences, though.
I think I can't do that in the top level make file very well (currently I
only
have the pkg-config check there for an early failure, but that check
doesn't
tell me if the library is present for all multilib variants). And I can't
check
for multilibs because I don't know if the bootstrap compiler is multilib
aware.
>>>
>>> hrm, so perhaps we need a —with-target-boehm-gc= type arrangement, and it’s
>>> the configurer’s responsibility to provide a path with appropriate
>>> headers/libs for the multi-lib configuration being attempted.
>>
>> I don't understand what you are proposing here.
>
> given that:
> auto-detection of the capabilities could be quite difficult (or, in the
> general case, unachievable) for the reasons you gave.
> the chosen target libraries might be in a non-standard place.
>
> making it an explicit requirement to point to them, and to ensure that the
> versions/multi-libs provided are suitable, (by putting
> —with-target-boehm-gc=/path/to/suitable/) would mean that the dependent
> configury (for objc-gc) could be just conditional upon the presence of the
> "with-target-boehm-gc”.
>
> I suppose that one could make "—with-target-boehm-gc” (no attached path)
> declare that the library (and requisite mult-lib versions) will be found in
> the sysroot without any additional work.
>
> The point here was to simplify the dependent configury so that it only needs
> to test something that the configuring user specifies (i.e. if they specify
> objc-gc, then they need also to specify the place that the gc lib can be
> found).
So here is the next proposal, I hope the added documentation in install.texi
makes the usage clear.
2016-11-19 Matthias Klose
* Makefile.def: Remove reference to boehm-gc target module.
* configure.ac: Include pkg.m4, check for --with-target-bdw-gc
options and for the bdw-gc pkg-config module.
* configure: Regenerate.
* Makefile.in: Regenerate.
gcc/
2016-11-19 Matthias Klose
* doc/install.texi: Document configure options --enable-objc-gc
and --with-target-bdw-gc.
config/
2016-11-19 Matthias Klose
* pkg.m4: New file.
libobjc/
2016-11-19 Matthias Klose
* configure.ac (--enable-objc-gc): Allow to configure with a
system provided boehm-gc.
* configure: Regenerate.
* Makefile.in (OBJC_BOEHM_GC_LIBS): Get value from configure.
* gc.c: Include system bdw-gc headers.
* memory.c: Likewise
* objects.c: Likewise
boehm-gc/
2016-11-19 Matthias Klose
Remove
2016-11-19 Matthias Klose
* Makefile.def: Remove reference to boehm-gc target module.
* configure.ac: Include pkg.m4, check for --with-target-bdw-gc
options and for the bdw-gc pkg-config module.
* configure: Regenerate.
* Makefile.in: Regenerate.
gcc/
2016-11-19 Matthias Klose
* doc/install.texi: Document configure options --enable-objc-gc
and --with-target-bdw-gc.
config/
2016-11-19 Matthias Klose
* pkg.m4: New file.
libobjc/
2016-11-19 Matthias Klose
* configure.ac (--enable-objc-gc): Allow to configure with a
system provided boehm-gc.
* configure: Regenerate.
* Makefile.in (OBJC_BOEHM_GC_LIBS): Get value from configure.
* gc.c: Include system bdw-gc headers.
* memory.c: Likewise
* objects.c: Likewise
boehm-gc/
2016-11-19 Matthias Klose
Remove
Index: Makefile.def
===
--- Makefile.def (revision 242638)
+++ Makefile.def (working copy)
@@ -166,7 +166,6 @@
target_modules = { module= libgloss; no_check=true; };
target_modules = { module= libffi; no_install=true; };
target_modules = { module= zlib; };
-target_modules = { module= boehm-gc; };
target_modules = { module= rda; };
target_modules = { module= libada; };
target_modules = { module= libgomp; bootstrap= true; lib_path=.libs; };
@@ -543,7 +542,6 @@
// a dependency on libgcc for native targets to configure.
lang_env_dependencies = { module=libiberty; no_c=true; };
-dependencies = { module=configure-target-boehm-gc; on=all-target-libstdc++-v3; };
dependencies = { module=configure-target-fastjar; on=configure-target-zlib; };
dependencies = { module=all-target-fastjar; on=all-target-zlib; };
dependencies = { module=configure-target-libgo; on=configure-target-libffi; };
@@ -551,8 +549,6 @@
dependencies = { module=all-target-libgo; on=all-target-libbacktrace; };
dependencies = { modul
Re: [RFC] Fix PR rtl-optimization/59461
>> This also eliminate quite a few zero-extensions in the compile.exp testsuite >> at -O2 on the SPARC. Tested on x86-64/Linux and SPARC/Solaris. >> >> >> 2016-11-07 Eric Botcazou >> >> PR rtl-optimization/59461 >> * doc/rtl.texi (paradoxical subregs): Add missing word. >> * combine.c (reg_nonzero_bits_for_combine): Do not discard results >> in modes with precision larger than that of last_set_mode. >> * rtlanal.c (nonzero_bits1) : If WORD_REGISTER_OPERATIONS is >> set and LOAD_EXTEND_OP is appropriate, propagate results from inner >> REGs to paradoxical SUBREGs. >> (num_sign_bit_copies1) : Likewise. Check that the mode is not >> larger than a word before invoking LOAD_EXTEND_OP on it. > > I have installed it after testing on ARM/EABI and IA-64/Linux. Just a heads-up for now, the above patch introduced following testsuite failures on Alpha [1]: FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O3 -g execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O3 -g execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test On a first look, it looks that something is wrong with sign-extensions. I will try to look into this a bit more. [1] https://gcc.gnu.org/ml/gcc-testresults/2016-11/msg02277.html Uros.
Re: [PATCH] avoid calling alloca(0)
On 11/19/2016 11:18 PM, Jakub Jelinek wrote: On Sat, Nov 19, 2016 at 04:43:29PM -0700, Martin Sebor wrote: Thanks for calling out the realloc(0, p) case! Realloc(0, p) is ?? The DR you refer to deprecates realloc(p, 0), not realloc(0, p). We are discussing zero size allocation so I was obviously referring to realloc(p, 0). Bernd had the arguments in the right order in his message and I transposed them by mistake in my reply. Martin
Re: [PATCH] avoid calling alloca(0)
On 11/20/2016 01:03 AM, Bernd Edlinger wrote:
On 11/20/16 00:43, Martin Sebor wrote:
As best I can tell the result isn't actually used (the code that
uses the result gets branched over). GCC just doesn't see it.
I verified this by changing the XALLOCAVEC macro to
#define XALLOCAVEC(T, N) (N ? alloca (sizeof (T) * N) : 0)
and bootstrapping and running the regression test suite with
no apparent regressions.
I would like this solution with braces around N better than
allocating one element when actually zero were requested.
The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.
I agree. Let me post an updated patch with the above fix and
leave it to others to choose which approach to go with.
you should also include glibc and busybox, to be sure.
Thanks for the suggestion. I've done that and found no instances
of any of these warnings in either Busybox 1.25.1 or Glibc trunk.
Good. I see many alloca calls all over, but mostly in the runtime
libraries, that don't use -Werror.
Have you done a bootstrap with all languages?
Were there warnings in the target libraries (note they don't use
-Werror, so you have to grep)?
Yes, I bootstrapped all languages. When testing my latest patch
yesterday I found a couple more places in GCC (one in Ada and
another in the middle end I think) that trigger the warning that
I had missed in the first patch. I'll post an updated patch soon.
I am a bit worried about that suggestion in libiberty/alloca.c:
"It is a good idea to use alloca(0) in
your main control loop, etc. to force garbage collection."
Because:
#include
#include
int main()
{
void* p;
int i;
for (i=0 ; i<100 ; i++)
{
p = alloca(0);
printf("%p\n", p);
}
}
... is fine, and allocates no memory.
But if I change that to alloca(1) the stack will blow up.
The suggestion to call alloca(0) in the main loop is to trigger
garbage collection when alloca is implemented using malloc (as
is apparently the case in some embedded environments). In that
case, alloca is not a built-in but a library function (i.e.,
-fno-builtin-alloca must be set) and the warning doesn't trigger.
This call to alloca(0) would also likely be made without using
the return value. When __builtin_alloca is called without using
the return value, the call is eliminated and the warning doesn't
trigger either.
Maybe it would be better to have a different warning option
for malloc/realloc with zero and for alloca with zero?
There is a -Walloca-larger-than that Aldy added earlier this
year. The option also diagnoses alloca(0), and it also warns
on calls to alloca in loops, but it's disabled by default.
I feel fairly strongly that all zero-length allocations
should be diagnosed at least with -Wextra but if that's what
it takes to move forward I'm willing to break things up this
way. If that's preferred then I would also expect to enable
-Walloca-larger-than=SIZE_MAX/2 (or smaller) by default, in
addition to having -Walloca-zero in -Wextra, analogously to
the corresponding warnings for the heap allocation functions.
(Ditto for -Wvla-larger-than=SIZE_MAX/2 and -Wvla-zero which
should probably also be added for consistency.)
Martin
Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)
On 11/20/2016 04:56 AM, Bernd Edlinger wrote:
Hi,
when you add a returns_nonnull to the builtin alloca
then this code in tree-vrp.c (gimple_stmt_nonzero_warnv_p)
should go away:
if (flag_delete_null_pointer_checks &&
lookup_attribute ("returns_nonnull",
TYPE_ATTRIBUTES (gimple_call_fntype (stmt
return true;
return gimple_alloca_call_p (stmt);
Yes, that looks like it should be superfluous now. Let me test
it. (It will make the null pointer test removal conditional on
flag_delete_null_pointer_checks but presumably that's okay.)
Regarding __builtin_alloca_with_align, I see no test cases
for that function:
__builtin_alloca_with_align is a bit special, because it allocates
more than the first parameter says, I think it allocates (if size != 0)
size + align/8 in order to be able to return an aligned object.
What if align is very large?
I'm guessing there are only a handful of tests for
__builtin_alloca_with_align because the built-in is used to allocate
VLAs and so it's exercised indirectly by VLA tests. There is one
fairly comprehensive test for the built-in that exercises the case
you mention: builtins-68.c, though perhaps not to the extent it
could or should. The problem is that there is no mechanism for
a compiler to indicate that a VLA has failed to allocate. So while
the following compiles (with my patch with a warning), it crashes
at runtime:
$ cat x.c && gcc -O2 -S x.c
void sink (void*);
void g (void)
{
struct S { int _Alignas (1024) i; };
unsigned long N = __SIZE_MAX__ / _Alignof (struct S);
struct S vla [N];
sink (vla);
}
x.c: In function ‘g’:
x.c:9:12: warning: argument 1 value ‘18446744073709550592ul’ exceeds
maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
struct S vla [N];
^~~
x.c:9:12: note: in a call to built-in allocation function
‘__builtin_alloca_with_align’
Or, with -Wvla-larger-than=1000 (or any other positive number):
x.c:9:12: warning: argument to variable-length array is too large
[-Wvla-larger-than=]
struct S vla [N];
^~~
x.c:9:12: note: limit is 1000 bytes, but argument is 18446744073709550592
I still would prefer separate -walloc-zero and -Walloca-zero
options. Likewise for the -Walloc-larger-than and -Walloca-larger-than
or better -Wmalloc-larger-than and -Walloca-larger-than ?
Sure, I'm open to it. But before I invest time into tweaking
things further I would like to get consensus on the two patches
I've posted (alloca(0) and this one), and ideally get them
at least tentatively approved. Otherwise, I would worry it
might be a wasted effort.
Martin
Re: [RFC] Fix PR rtl-optimization/59461
On Sun, Nov 20, 2016 at 10:00 PM, Uros Bizjak wrote: >>> This also eliminate quite a few zero-extensions in the compile.exp testsuite >>> at -O2 on the SPARC. Tested on x86-64/Linux and SPARC/Solaris. >>> >>> >>> 2016-11-07 Eric Botcazou >>> >>> PR rtl-optimization/59461 >>> * doc/rtl.texi (paradoxical subregs): Add missing word. >>> * combine.c (reg_nonzero_bits_for_combine): Do not discard results >>> in modes with precision larger than that of last_set_mode. >>> * rtlanal.c (nonzero_bits1) : If WORD_REGISTER_OPERATIONS is >>> set and LOAD_EXTEND_OP is appropriate, propagate results from inner >>> REGs to paradoxical SUBREGs. >>> (num_sign_bit_copies1) : Likewise. Check that the mode is not >>> larger than a word before invoking LOAD_EXTEND_OP on it. >> >> I have installed it after testing on ARM/EABI and IA-64/Linux. > > Just a heads-up for now, the above patch introduced following > testsuite failures on Alpha [1]: > > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O3 -g execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 -flto > -fno-use-linker-plugin -flto-partition=none execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O3 -g execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 -flto > -fno-use-linker-plugin -flto-partition=none execution test > FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects execution test > > On a first look, it looks that something is wrong with sign-extensions. > > I will try to look into this a bit more. Your patch exposes a problem in the ree pass where: 50: $22:QI=$8:QI REG_EQUAL [`expected'] 43: [$2:DI+low(`expected')]=$8:QI 51: $3:DI=sign_extend($22:QI) gets converted to: 50: $3:DI=sign_extend($8:QI) 141: $22:DI=$3:DI 43: [$2:DI+low(`expected')]=$8:QI $22:DI is used later in a fallthrough block, and the above sequences are certainly not equal wrt. to $22. The first one does have all high bits of $22 zero, the second one doesn't. -fno-ree fixes the failures. I will fill a PR tomorrow. Uros.
Re: [RFC] Fix PR rtl-optimization/59461
On Mon, Nov 21, 2016 at 12:49 AM, Uros Bizjak wrote: > On Sun, Nov 20, 2016 at 10:00 PM, Uros Bizjak wrote: This also eliminate quite a few zero-extensions in the compile.exp testsuite at -O2 on the SPARC. Tested on x86-64/Linux and SPARC/Solaris. 2016-11-07 Eric Botcazou PR rtl-optimization/59461 * doc/rtl.texi (paradoxical subregs): Add missing word. * combine.c (reg_nonzero_bits_for_combine): Do not discard results in modes with precision larger than that of last_set_mode. * rtlanal.c (nonzero_bits1) : If WORD_REGISTER_OPERATIONS is set and LOAD_EXTEND_OP is appropriate, propagate results from inner REGs to paradoxical SUBREGs. (num_sign_bit_copies1) : Likewise. Check that the mode is not larger than a word before invoking LOAD_EXTEND_OP on it. >>> >>> I have installed it after testing on ARM/EABI and IA-64/Linux. >> >> Just a heads-up for now, the above patch introduced following >> testsuite failures on Alpha [1]: >> >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O3 -g execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 -flto >> -fno-use-linker-plugin -flto-partition=none execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-1.c -O2 -flto >> -fuse-linker-plugin -fno-fat-lto-objects execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O3 -g execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 -flto >> -fno-use-linker-plugin -flto-partition=none execution test >> FAIL: gcc.dg/atomic/stdatomic-compare-exchange-2.c -O2 -flto >> -fuse-linker-plugin -fno-fat-lto-objects execution test >> >> On a first look, it looks that something is wrong with sign-extensions. >> >> I will try to look into this a bit more. > > Your patch exposes a problem in the ree pass where: > >50: $22:QI=$8:QI > REG_EQUAL [`expected'] >43: [$2:DI+low(`expected')]=$8:QI >51: $3:DI=sign_extend($22:QI) > > gets converted to: > >50: $3:DI=sign_extend($8:QI) > 141: $22:DI=$3:DI >43: [$2:DI+low(`expected')]=$8:QI > > $22:DI is used later in a fallthrough block, and the above sequences > are certainly not equal wrt. to $22. The first one does have all high > bits of $22 zero, the second one doesn't. > > -fno-ree fixes the failures. > > I will fill a PR tomorrow. PR 78437 [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78437 Uros.
[PR target/48551] Make double_reg_address_ok a per-mode test
Sigh. Reload.
Reload has the assumption that if reg+reg addressing is valid in QImode
that it is valid in all modes. This has been baked into reload as long
as I can remember.
It's part of a larger problem that reload doesn't handle some cases
where address validity is dependent upon the context in which the
address is used. I'm not going to try to solve that problem as a whole,
but I can make this tiny piece better.
In BZ48551 we have a case where we want to reload an address like
(plus (fp) (-largeint)) for a DFmode memory access. We need the reload
because the target has a limited range for displacements.
Reload has multiple strategies for handling this situation, including
loading the integer into an index register and using reg+reg style
addressing. This case is predicated on checking double_reg_address_ok.
The problem is when we initialize double_reg_address_ok, we only do so
for QImode. So if the target only allows reg+reg style addressing in
QImode, but not other modes modes, then we can end up generating bogus
RTL leading to an insn recognition failure as seen in this BZ as well as
building libgfortran on m68k-elf.
The easy solution here is to have double_reg_address_ok for all modes.
In this particular case that will cause reload to compute (plus (fp)
-largeint) into an address register, then use simple register indirect.
This should be safe on other ports still using reload. If they have
properties similar to coldfire, then this could potentially avoid the
same problem on those ports. If they were the opposite (reg+reg not
allowed for QImode, but is allowed in other modes), then this could
allows those ports to generate slightly more efficient code. Either way
it seems like the right thing to do.
This fixes BZ 48551 as well as the libgfortran build failures for
m68k-elf. Another win for retro-computing. Verified against the
testcase in BZ48551 as well as building libgfortran for m68k-elf.
Then I went back to gcc-4.7, which is pre-lra and did a bootstrap and
comparison test on x86_64-linux-gnu to exercise the code path a little
harder. That (of course) was successful.
Installing on the trunk.
Jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 91cbe82..40d6f8a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2016-11-20 Jeff Law
+
+ PR target/48551
+ * reload.h (struct target_reload): Make x_double_reg_address_ok
+ be per-mode rather.
+ * reload.c (find_reloads_address): Check if double_reg_address_ok
+ is true for the mode of the memory reference.
+ * reload1.c (init_reload): Initialize double_reg_address_ok for
+ each mode.
+
2016-11-20 Aldy Hernandez
PR middle-end/61409
diff --git a/gcc/reload.c b/gcc/reload.c
index 7d16817..4cba220 100644
--- a/gcc/reload.c
+++ b/gcc/reload.c
@@ -5090,7 +5090,7 @@ find_reloads_address (machine_mode mode, rtx *memrefloc,
rtx ad,
loc = &XEXP (*loc, 0);
}
- if (double_reg_address_ok
+ if (double_reg_address_ok[mode]
&& regno_ok_for_base_p (REGNO (XEXP (ad, 0)), mode, as,
PLUS, CONST_INT))
{
diff --git a/gcc/reload.h b/gcc/reload.h
index 98b75e3..1fc8ecb 100644
--- a/gcc/reload.h
+++ b/gcc/reload.h
@@ -159,9 +159,6 @@ struct target_reload {
which these are valid is the same as spill_indirect_levels, above. */
bool x_indirect_symref_ok;
- /* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
- bool x_double_reg_address_ok;
-
/* Nonzero if indirect addressing is supported on the machine; this means
that spilling (REG n) does not require reloading it into a register in
order to do (MEM (REG n)) or (MEM (PLUS (REG n) (CONST_INT c))). The
@@ -181,6 +178,10 @@ struct target_reload {
[FIRST_PSEUDO_REGISTER]
[MAX_MOVE_MAX / MIN_UNITS_PER_WORD + 1]);
+ /* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid
+ in the given mode. */
+ bool x_double_reg_address_ok[MAX_MACHINE_MODE];
+
/* We will only make a register eligible for caller-save if it can be
saved in its widest mode with a simple SET insn as long as the memory
address is valid. We record the INSN_CODE is those insns here since
diff --git a/gcc/reload1.c b/gcc/reload1.c
index 4ee3840..b855268 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -448,11 +448,10 @@ init_reload (void)
/* This way, we make sure that reg+reg is an offsettable address. */
tem = plus_constant (Pmode, tem, 4);
- if (memory_address_p (QImode, tem))
- {
- double_reg_address_ok = 1;
- break;
- }
+ for (int mode = 0; mode < MAX_MACHINE_MODE; mode++)
+ if (!double_reg_address_ok[mode]
+ && memory_address_p ((enum machine_mode)mode, tem))
+ double_reg_address_ok[mode] = 1;
}
/* Initialize obstack for our rtl allocat
Re: [PATCH] Delete GCJ
On 11/20/2016 01:42 PM, Matthias Klose wrote:
+The options @option{--with-target-bdw-gc-include} and
+@option{--with-target-bdw-gc-include} must always specified together for
+each multilib variant and take precedence over
+@option{--with-target-bdw-gc-include}. If none of these options are
+specified, the values are taken from the @command{pkg-config}
+@samp{bdw-gc} module.
This paragraph makes no sense to me. :-(
-Sandra
Re: [PATCH] [AArch64] Fix PR78382
Hi Kugan,
>> Why don't you use the mode of dest as done in other similar places. Like:
Thanks for the pointer. Modified the patch as per your suggestion.
Please find attached the modified patch and let me know your comments.
Bootstrapped and regression tested on Thunderx.
Thanks,
Naveen
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index bd97c5b..4aea578 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1374,10 +1374,14 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm,
case SYMBOL_SMALL_TLSGD:
{
rtx_insn *insns;
- rtx result = gen_rtx_REG (Pmode, R0_REGNUM);
+ machine_mode mode = GET_MODE (dest);
+ rtx result = gen_rtx_REG (mode, R0_REGNUM);
start_sequence ();
- aarch64_emit_call_insn (gen_tlsgd_small (result, imm));
+ if (TARGET_ILP32)
+ aarch64_emit_call_insn (gen_tlsgd_small_si (result, imm));
+ else
+ aarch64_emit_call_insn (gen_tlsgd_small_di (result, imm));
insns = get_insns ();
end_sequence ();
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index a652a7c..4833c7f 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -5089,20 +5089,20 @@
;; The TLS ABI specifically requires that the compiler does not schedule
;; instructions in the TLS stubs, in order to enable linker relaxation.
;; Therefore we treat the stubs as an atomic sequence.
-(define_expand "tlsgd_small"
+(define_expand "tlsgd_small_"
[(parallel [(set (match_operand 0 "register_operand" "")
(call (mem:DI (match_dup 2)) (const_int 1)))
- (unspec:DI [(match_operand:DI 1 "aarch64_valid_symref" "")] UNSPEC_GOTSMALLTLS)
+ (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref" "")] UNSPEC_GOTSMALLTLS)
(clobber (reg:DI LR_REGNUM))])]
""
{
operands[2] = aarch64_tls_get_addr ();
})
-(define_insn "*tlsgd_small"
+(define_insn "*tlsgd_small_"
[(set (match_operand 0 "register_operand" "")
(call (mem:DI (match_operand:DI 2 "" "")) (const_int 1)))
- (unspec:DI [(match_operand:DI 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS)
+ (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS)
(clobber (reg:DI LR_REGNUM))
]
""
diff --git a/gcc/testsuite/gcc.target/aarch64/pr78382.c b/gcc/testsuite/gcc.target/aarch64/pr78382.c
new file mode 100644
index 000..6c98e5e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr78382.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O0 -fpic -mabi=ilp32 -mtls-dialect=trad" } */
+
+__thread int abc;
+void
+foo ()
+{
+ int *p;
+ p = &abc;
+}
Re: [PATCHv2, C++] Warn on redefinition of builtin functions (PR c++/71973)
OK.
On Sat, Nov 19, 2016 at 6:11 AM, Bernd Edlinger
wrote:
> Hi,
>
> On 11/18/16 22:19, Jason Merrill wrote:
>> On 11/05/2016 12:44 PM, Bernd Edlinger wrote:
>>> + warning_at (DECL_SOURCE_LOCATION (newdecl), 0,
>>> + "declaration of %q+#D conflicts with built-in "
>>> + "declaration %q#D", newdecl, olddecl);
>>
>> There needs to be a way to disable this warning, even if it's enabled by
>> default.
>>
>>> - TREE_NOTHROW (olddecl) = 0;
>>> + TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl);
>>
>> I still think a better fix would be to add a copy of TREE_NOTHROW to the
>> else block of the if (types_match), to go with the existing copies of
>> TREE_READONLY and TREE_THIS_VOLATILE.
>>
>
>
> I changed the patch as requested. I think meanwhile that this else
> block does only handle a built-in function with different signature.
>
> I have now changed also the C front end to emit the warning with the new
> default-enabled -Wbuiltin-declaration-mismatch.
>
> When I looked at the c-decl.c code, I saw there are effectively two
> warnings, one for a function that does not match the builtin signature,
> and another for a global variable that has the the name of a built-in.
>
> like:
> test.c
> int printf;
>
> gives warning:
> test.c:1:5: warning: built-in function 'printf' declared as non-function
> int printf;
>
>
> the same in C++ gives nothing.
>
> That is because of this in duplicate_decls:
>
>if (TREE_CODE (newdecl) != FUNCTION_DECL)
> {
>/* Avoid warnings redeclaring built-ins which have not been
> explicitly declared. */
>if (DECL_ANTICIPATED (olddecl))
> return NULL_TREE;
>
> That is from gcc3.1 in 2002 a rather radical patch I would say.
>
> I left the warning for declaring a variable with the name of built-in
> function for next time, maybe...
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
Re: [patch,avr] Disallow LDS / STS for .rodata on AVR_TINY.
2016-11-18 16:31 GMT+03:00 Georg-Johann Lay : > Currently, Binutils still comes with an AVR_TINY linker description file > which puts .rodata into RAM so that LDS is applicable for reading such > objects. > > However, as these devices come with a linearised address model, linker > descriptions which put .rodata into flash are much more reasonable. This > patch caters for such situations: As .rodata virtual addresses will be > offset by 0x4000, respective objects may no more be accessed by LDS. > > Moreover, objects with a section attribute won't be accessed by LDS. > > Ok for trunk? > > Johann > > PR target/78093 > * config/avr/avr.c (avr_decl_maybe_lds_p): New static function. > (avr_encode_section_info) [TARGET_ABSDATA && AVR_TINY]: Use it. Approved. Please apply.
