Re: [PATCH 2/2] Documentation hyperlinks for [-Wname-of-option] (PR 87488)
Hi David,
I just wanted to say that this does not work right on ubuntu 14.04 at least:
g++ -Wshadow=local -Wno-shadow=compatible-local -S Wshadow-1.c
Wshadow-1.c: In function 'void foo(int*, int*)':
Wshadow-1.c:8:9: warning: declaration of 'int d' shadows a parameter
[]8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local-Wshadow=local]8;;]
8 | int d = 0; /* { dg-warning "Wshadow=local" } */
| ^
Wshadow-1.c:4:23: note: shadowed declaration is here
4 | void foo(int *c, int *d) /* { dg-bogus "Wshadow" } */
| ~^
Wshadow-1.c: In function 'void bar(int*, int*)':
Wshadow-1.c:15:15: warning: declaration of 'c' shadows a global declaration
[]8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8;;]
15 | void bar(int *c, int *d) /* { dg-warning "Wshadow" } */
| ~^
Wshadow-1.c:3:5: note: shadowed declaration is here
3 | int c;
| ^
Wshadow-1.c:19:9: warning: declaration of 'int d' shadows a parameter
[]8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8;;]
19 | int d = 0; /* { dg-warning "Wshadow" } */
| ^
Wshadow-1.c:15:23: note: shadowed declaration is here
15 | void bar(int *c, int *d) /* { dg-warning "Wshadow" } */
| ~^
Wshadow-1.c:22:10: warning: declaration of 'e' shadows a previous local
[]8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8;;]
22 | int *e = 0;/* { dg-warning "Wshadow" } */
| ^
Wshadow-1.c:17:8: note: shadowed declaration is here
17 | int *e = d;
|^
Coloring works, even the hyperlinks seem to work, when clicked at, although
they jump just
to the top of the page, and one has to scroll down to the warning manually.
But the warning name is very hard to spot in all that glibberish long text :-(
Do you see a way to find out if the escape sequences are supported,
or how to disable this function per configure option?
Thanks
Bernd.
[PATCH target/92035] Add missing avx512f intrinsics
Hi:
This patch is enabling missing avx512f intrinsics listed as
_mm_mask_roundscale_sd
_mm_mask_roundscale_round_sd
_mm_maskz_roundscale_sd
_mm_maskz_roundscale_round_sd
_mm_mask_roundscale_ss
_mm_mask_roundscale_round_ss
_mm_maskz_roundscale_ss
_mm_maskz_roundscale_round_ss
Bootstrap ok, regression tests for i386/x86 ok.
ChangeLog
gcc/
* config/i386/avx512fintrin.h (_mm_mask_roundscale_ss,
_mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
_mm_maskz_roundscale_round_ss, _mm_mask_roundscale_sd,
_mm_maskz_roundscale_sd, _mm_mask_roundscale_round_sd,
_mm_maskz_roundscale_round_sd): New intrinsics.
(_mm_roundscale_ss, _mm_roundscale_round_ss): Fix.
* config/i386/i386-builtin.def (__builtin_ia32_rndscaless_round,
__builtin_ia32_rndscalesd_round): Remove.
(__builtin_ia32_rndscalesd_mask_round,
__builtin_ia32_rndscalesd_mask_round): New intrinsics.
* config/i386/sse.md
(avx512f_rndscale): Renamed to ...
(avx512f_rndscale):
... this.
((match_operand:VF_128 2 ""
"")): Changed to ...
((match_operand:VF_128 2 ""
"")): ... this.
("vrndscale\t{%3, %2, %1,
%0|%0, %1, %2, %3}"): Changed to ...
("vrndscale\t{%3,%2,
%1,
%0|%0, %1,
%2, %3}"): ... this.
gcc/testsuite/
* gcc.target/i386/avx512f-vrndscaless-1.c (_mm_mask_roundscale_ss,
_mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
_mm_maskz_roundscale_round_ss): Test new intrinsics.
* gcc.target/i386/avx512f-vrndscaless-2.c (_mm_mask_roundscale_ss,
_mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
_mm_maskz_roundscale_round_ss): Test new intrinsics.
* gcc.target/i386/avx512f-vrndscalesd-1.c (_mm_mask_roundscale_sd,
_mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
_mm_maskz_roundscale_round_sd): Test new intrinsics.
* gcc.target/i386/avx512f-vrndscalesd-2.c (_mm_mask_roundscale_sd,
_mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
_mm_maskz_roundscale_round_sd): Test new intrinsics.
* gcc.target/i386/avx-1.c (__builtin_ia32_rndscalefss_round,
__builtin_ia32_rndscalefsd_round): Remove builtin.
(__builtin_ia32_rndscalefss_mask_round,
__builtin_ia32_rndscalefsd_mask_round): Test new builtin.
* gcc.target/i386/sse-13.c: Ditto.
* gcc.target/i386/sse-23.c: Ditto.
--
BR,
Hongtao
From 39a2547f73c63493d502384c45b38b3dc54005c8 Mon Sep 17 00:00:00 2001
From: "Wang, Hongyu"
Date: Sat, 12 Oct 2019 00:07:01 -0700
Subject: [PATCH] PR target/92035
Add missing mask[z]_roundscale_[round]_s[d,s] intrinsics
gcc/
* config/i386/avx512fintrin.h (_mm_mask_roundscale_ss,
_mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
_mm_maskz_roundscale_round_ss, _mm_mask_roundscale_sd,
_mm_maskz_roundscale_sd, _mm_mask_roundscale_round_sd,
_mm_maskz_roundscale_round_sd): New intrinsics.
(_mm_roundscale_ss, _mm_roundscale_round_ss): Fix.
* config/i386/i386-builtin.def (__builtin_ia32_rndscaless_round,
__builtin_ia32_rndscalesd_round): Remove.
(__builtin_ia32_rndscalesd_mask_round,
__builtin_ia32_rndscalesd_mask_round): New intrinsics.
* config/i386/sse.md (avx512f_rndscale): Renamed to ...
(avx512f_rndscale): ... this.
((match_operand:VF_128 2 ""
"")): Changed to ...
((match_operand:VF_128 2 ""
"")): ... this.
("vrndscale\t{%3, %2, %1,
%0|%0, %1, %2, %3}"): Changed to ...
("vrndscale\t{%3,%2, %1,
%0|%0, %1,
%2, %3}"): ... this.
gcc/testsuite/
* gcc.target/i386/avx512f-vrndscaless-1.c (_mm_mask_roundscale_ss,
_mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
_mm_maskz_roundscale_round_ss): Test new intrinsics.
* gcc.target/i386/avx512f-vrndscaless-2.c (_mm_mask_roundscale_ss,
_mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
_mm_maskz_roundscale_round_ss): Test new intrinsics.
* gcc.target/i386/avx512f-vrndscalesd-1.c (_mm_mask_roundscale_sd,
_mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
_mm_maskz_roundscale_round_sd): Test new intrinsics.
* gcc.target/i386/avx512f-vrndscalesd-2.c (_mm_mask_roundscale_sd,
_mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
_mm_maskz_roundscale_round_sd): Test new intrinsics.
* gcc.target/i386/avx-1.c (__builtin_ia32_rndscalefss_round,
__builtin_ia32_rndscalefsd_round): Remove builtin.
(__builtin_ia32_rndscalefss_mask_round,
__builtin_ia32_rndscalefsd_mask_round): Test new builtin.
* gcc.target/i386/sse-13.c: Ditto.
* gcc.target/i386/sse-23.c: Ditto.
---
gcc/config/i386/avx512fintrin.h | 234 --
gcc/config/i386/i386-builtin.def | 4 +-
gcc/config/i386/sse.md| 9 +-
gcc/testsuite/gcc.target/i386/avx-1.c | 4 +-
.../gcc.target/i386/avx512f-vrndscalesd-1.c | 12 +-
.../gcc.target/i386/avx512f-vrndscalesd-2.c | 42 +++-
.../gcc.target/i386/avx512f-vrndscaless-1.c | 12 +-
.
Re: [PATCH] PR fortran/91513 -- Fix poorly worded error message
On Sat, Oct 12, 2019 at 7:58 AM Steve Kargl wrote: > > On Fri, Oct 11, 2019 at 09:56:15PM -0700, Steve Kargl wrote: > > In PR fortran/91513, Damian Rouson points out that the Fortran > > does contain the words "impure variable". > > Geez. That is a messed up sentence. > > In PR fortran/91513, Damian Rouson points out that the Fortran > *standard* does *not* contain the words "impure variable. > > (It's late, and I'm tired!) > > > Damian and I spent > > an afternoon together a few weeks ago where I gave Damian a > > world wind view of how I see gcc/fortran and go about debugging > > problems. Damian and I both have full schedules, but I was > > hoping he would submit the patch to the list. My time of > > fixing gfortran bugs is running out, so ... > > > > OK to commit? > > > > 2019-09-29 Damian Rouson > > > > PR fortran/91513 > > * resolve.c (resolve_ordinary_assign): Improved error message. > > > > 2019-09-29 Damian Rouson > > > > PR fortran/91513 > > * gfortran.dg/impure_assignment_2.f90: Update dg-error regex. > > > > -- > > Steve Ok -- Janne Blomqvist
Re: [PATCH target/92035] Add missing avx512f intrinsics
Hi!
> gcc/
> * config/i386/avx512fintrin.h (_mm_mask_roundscale_ss,
> _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> _mm_maskz_roundscale_round_ss, _mm_mask_roundscale_sd,
> _mm_maskz_roundscale_sd, _mm_mask_roundscale_round_sd,
> _mm_maskz_roundscale_round_sd): New intrinsics.
> (_mm_roundscale_ss, _mm_roundscale_round_ss): Fix.
"Fix." doesn't describe the change you've done. So I think it should be
instead:
"Use __builtin_ia32_rndscales?_mask_round builtins instead of
__builtin_ia32_rndscales?_round."
> * config/i386/i386-builtin.def (__builtin_ia32_rndscaless_round,
> __builtin_ia32_rndscalesd_round): Remove.
> (__builtin_ia32_rndscalesd_mask_round,
Pasto, sd listed twice, ss not listed, change the first one to ss.
> __builtin_ia32_rndscalesd_mask_round): New intrinsics.
> * config/i386/sse.md (avx512f_rndscale):
> Renamed to ...
> (avx512f_rndscale):
> ... this.
These two lines are too long. Perhaps:
* config/i386/sse.md
(avx512f_rndscale): Renamed to ...
(avx512f_rndscale):
... this.
> ((match_operand:VF_128 2 ""
> "")): Changed to ...
> ((match_operand:VF_128 2 ""
> "")): ... this.
> ("vrndscale\t{%3, %2, %1,
> %0|%0, %1, %2, %3}"): Changed to ...
>
> ("vrndscale\t{%3,%2, %1,
> %0|%0, %1,
> %2, %3}"): ... this.
But the above is not appropriate, the ChangeLog in *.md is at the level of
define_{insn,expand,split,peephole2,insn_and_split} etc., not at the level
of individual subrtls or patterns.
So, the right thing is just to ammend the "... this.", follow it up by a
sentence what also changed. Like " Adjust and add subst attributes to make
it maskable."
>
> gcc/testsuite/
> * gcc.target/i386/avx512f-vrndscaless-1.c (_mm_mask_roundscale_ss,
> _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> _mm_maskz_roundscale_round_ss): Test new intrinsics.
That is again not what you've changed. For tests, often the exact
spots aren't listed and one just uses *.c: Description. but if you want to
use details, you can e.g.
* gcc.target/i386/avx512f-vrndscaless-1.c: Add scan-assembler-times
directives for newly expected instructions.
(m): New variable.
(avx512f_test): Add tests for new intrinsics.
> * gcc.target/i386/avx512f-vrndscaless-2.c (_mm_mask_roundscale_ss,
> _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> _mm_maskz_roundscale_round_ss): Test new intrinsics.
> * gcc.target/i386/avx512f-vrndscalesd-1.c (_mm_mask_roundscale_sd,
> _mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
> _mm_maskz_roundscale_round_sd): Test new intrinsics.
> * gcc.target/i386/avx512f-vrndscalesd-2.c (_mm_mask_roundscale_sd,
> _mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
> _mm_maskz_roundscale_round_sd): Test new intrinsics.
Likewise.
> * gcc.target/i386/avx-1.c (__builtin_ia32_rndscalefss_round,
> __builtin_ia32_rndscalefsd_round): Remove builtin.
> (__builtin_ia32_rndscalefss_mask_round,
> __builtin_ia32_rndscalefsd_mask_round): Test new builtin.
That is not what you've changed. You are there not Removing a builtin
and testing a new builtin, but removing a macro and defining a new macro.
So I think
: Remove.
: Define.
is more appropriate.
> +#define _mm_roundscale_round_ss(A, B, I, R)
> \
> + ((__m128) __builtin_ia32_rndscaless_mask_round ((__v4sf)(__m128)(A),
> \
> + (__v4sf)(__m128)(B), \
> + (int)(I), \
> + (__v4sf)_mm_setzero_ps(),\
There should be a space after _mm_setzero_p[sd] .
I know the formatting of many macros is just wrong, but no need to add more
of that.
Ok for trunk with those nits fixed.
In fact, it would be probably cleaner to:
((__m128) \
__builtin_ia32_rndscaless_mask_round ((__v4sf) (__m128) (A), \
(__v4sf) (__m128) (B), \
(int) (I), \
(__v4sf) _mm_setzero_ps (),\
(__mmask8) (-1), \
(int) (R))
or so, because then one has for long builtin names more space. But
I'm not asking for that to be changed.
Jakub
[committed] Further work on declare variant
Hi!
The following patch adds diagnostics if the same function serves as variant
for multiple base functions and there is a mismatch in the construct
selector set. Additionally, it adds a start of a function that says if a
context selector matches the OpenMP context, doesn't or we don't know yet.
In the last case we need to defer the decision until later.
The C++ part for this isn't done, because I need to figure out the right way
to perform the lookup (using dummy arguments (and dummy instance for
methods) based on the function argument types and method context)).
Bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk.
2019-10-12 Jakub Jelinek
c-family/
* c-common.h (c_omp_mark_declare_variant,
c_omp_context_selector_matches): Declare.
* c-omp.c: Include attribs.h, gimplify.h, cgraph.h, symbol-summary.h
and hsa-common.h.
(c_omp_get_context_selector): Support second argument NULL.
(c_omp_mark_declare_variant, c_omp_context_selector_matches): New
functions.
* c-attribs.c (c_common_attribute_table): Remove "omp declare variant"
attribute, add "omp declare variant base" and
"omp declare variant variant" attributes.
c/
* c-parser.c (c_parser_omp_context_selector): Improve error recovery.
For simd properties, put them directly into TREE_VALUE.
(c_finish_omp_declare_variant): Call c_omp_mark_declare_variant.
If c_omp_context_selector_matches is 0, don't add attribute, otherwise
add "omp declare variant base" attribute rather than
"omp declare variant".
cp/
* parser.c (cp_parser_omp_context_selector): Improve error recovery.
For simd properties, put them directly into TREE_VALUE.
(cp_finish_omp_declare_variant): Add "omp declare variant base"
attribute rather than "omp declare variant".
testsuite/
* c-c++-common/gomp/declare-variant-2.c: Adjust for error recovery
improvements. Add new tests.
* c-c++-common/gomp/declare-variant-4.c: New test.
* c-c++-common/gomp/declare-variant-5.c: New test.
* c-c++-common/gomp/declare-variant-6.c: New test.
* c-c++-common/gomp/declare-variant-7.c: New test.
--- gcc/c-family/c-common.h.jj 2019-10-11 20:48:23.823186584 +0200
+++ gcc/c-family/c-common.h 2019-10-11 20:48:36.795991404 +0200
@@ -1191,6 +1191,8 @@ extern bool c_omp_predefined_variable (t
extern enum omp_clause_default_kind c_omp_predetermined_sharing (tree);
extern tree c_omp_check_context_selector (location_t, tree);
extern tree c_omp_get_context_selector (tree, const char *, const char *);
+extern void c_omp_mark_declare_variant (location_t, tree, tree);
+extern int c_omp_context_selector_matches (tree);
/* Return next tree in the chain for chain_next walking of tree nodes. */
static inline tree
--- gcc/c-family/c-omp.c.jj 2019-10-11 20:48:23.812186749 +0200
+++ gcc/c-family/c-omp.c2019-10-11 21:07:33.083904116 +0200
@@ -32,6 +32,11 @@ along with GCC; see the file COPYING3.
#include "omp-general.h"
#include "gomp-constants.h"
#include "memmodel.h"
+#include "attribs.h"
+#include "gimplify.h"
+#include "cgraph.h"
+#include "symbol-summary.h"
+#include "hsa-common.h"
/* Complete a #pragma oacc wait construct. LOC is the location of
@@ -2236,17 +2241,335 @@ c_omp_check_context_selector (location_t
}
/* From context selector CTX, return trait-selector with name SEL in
- trait-selector-set with name SET if any, or NULL_TREE if not found. */
+ trait-selector-set with name SET if any, or NULL_TREE if not found.
+ If SEL is NULL, return the list of trait-selectors in SET. */
tree
c_omp_get_context_selector (tree ctx, const char *set, const char *sel)
{
tree setid = get_identifier (set);
- tree selid = get_identifier (sel);
+ tree selid = sel ? get_identifier (sel) : NULL_TREE;
for (tree t1 = ctx; t1; t1 = TREE_CHAIN (t1))
if (TREE_PURPOSE (t1) == setid)
- for (tree t2 = TREE_VALUE (t1); t2; t2 = TREE_CHAIN (t2))
- if (TREE_PURPOSE (t2) == selid)
- return t2;
+ {
+ if (sel == NULL)
+ return TREE_VALUE (t1);
+ for (tree t2 = TREE_VALUE (t1); t2; t2 = TREE_CHAIN (t2))
+ if (TREE_PURPOSE (t2) == selid)
+ return t2;
+ }
return NULL_TREE;
}
+
+/* Register VARIANT as variant of some base function marked with
+ #pragma omp declare variant. CONSTRUCT is corresponding construct
+ selector set. */
+
+void
+c_omp_mark_declare_variant (location_t loc, tree variant, tree construct)
+{
+ tree attr = lookup_attribute ("omp declare variant variant",
+ DECL_ATTRIBUTES (variant));
+ if (attr == NULL_TREE)
+{
+ attr = tree_cons (get_identifier ("omp declare variant variant"),
+ unshare_expr (construct),
+ DECL_ATTRIBUTES (variant));
+ DECL_ATTRIBUTES (variant) = attr;
+ return;
+
[PATCH] Fix up *COND_EXPR *trap* handling (PR middle-end/92063)
Hi!
As mentioned in the PR and on IRC, tree_could_trap_p is described
as taking GIMPLE expressions only, but in fact we rely on it not crashing
when feeded GENERIC, just for GENERIC it will not handle expressions
recursively and we have generic_expr_could_trap_p for that that calls
tree_could_trap_p recursively.
The addition of != COND_EXPR and != VEC_COND_EXPR assert to
operation_could_trap_p broke this, because if GENERIC with COND_EXPR
being first argument (condition) of another COND_EXPR is fed to
tree_could_trap_p, it now ICEs.
The following patch fixes it by:
1) in tree_could_trap_p return false for {,VEC_}COND_EXPR rather than
just recursing on the first argument. For GIMPLE, we shouldn't be called
with {,VEC_}COND_EXPR, because those are ternary rhs and thus 3 arguments,
not one. For GENERIC, we can, but we also need to recurse and the recursion
should discover that the comparison may trap.
2) in simple_operand_p_2 which calls tree_could_trap_p on GENERIC, this is
changed to generic_expr_could_trap_p so that it actually recurses and tests
subexpressions too
3) in operation_could_trap_helper_p signals that *COND_EXPR is not handled
and it doesn't care about whether *COND_EXPR is floating point or not.
This change is for sccvn, which calls operation_could_trap_helper_p
and then, if not handled, calls tree_could_trap_p on the operands.
Without the first hunk, *COND_EXPR is handled by the default handling,
which says that fp_operation can trap (but *COND_EXPR with fp operands can't
in itself) and makes it unhandled otherwise, at which point we call
tree_could_trap_p on the condition, which is all that matters.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2019-10-12 Jakub Jelinek
PR middle-end/92063
* tree-eh.c (operation_could_trap_helper_p)
: Return false with *handled = false.
(tree_could_trap_p): For {,VEC_}COND_EXPR return false instead of
recursing on the first operand.
* fold-const.c (simple_operand_p_2): Use generic_expr_could_trap_p
instead of tree_could_trap_p.
* tree-ssa-sccvn.c (vn_nary_may_trap): Formatting fixes.
* gcc.c-torture/compile/pr92063.c: New test.
--- gcc/tree-eh.c.jj2019-10-07 17:30:31.028153702 +0200
+++ gcc/tree-eh.c 2019-10-11 13:46:12.626811039 +0200
@@ -2499,6 +2499,14 @@ operation_could_trap_helper_p (enum tree
/* Constructing an object cannot trap. */
return false;
+case COND_EXPR:
+case VEC_COND_EXPR:
+ /* Whether *COND_EXPR can trap depends on whether the
+first argument can trap, so signal it as not handled.
+Whether lhs is floating or not doesn't matter. */
+ *handled = false;
+ return false;
+
default:
/* Any floating arithmetic may trap. */
if (fp_operation && flag_trapping_math)
@@ -2614,9 +2622,12 @@ tree_could_trap_p (tree expr)
if (!expr)
return false;
- /* For COND_EXPR and VEC_COND_EXPR only the condition may trap. */
+ /* In COND_EXPR and VEC_COND_EXPR only the condition may trap, but
+ they won't appear as operands in GIMPLE form, so this is just for the
+ GENERIC uses where it needs to recurse on the operands and so
+ *COND_EXPR itself doesn't trap. */
if (TREE_CODE (expr) == COND_EXPR || TREE_CODE (expr) == VEC_COND_EXPR)
-expr = TREE_OPERAND (expr, 0);
+return false;
code = TREE_CODE (expr);
t = TREE_TYPE (expr);
--- gcc/fold-const.c.jj 2019-10-09 10:27:12.578402783 +0200
+++ gcc/fold-const.c2019-10-11 12:29:53.426603712 +0200
@@ -4447,8 +4447,7 @@ simple_operand_p_2 (tree exp)
{
enum tree_code code;
- if (TREE_SIDE_EFFECTS (exp)
- || tree_could_trap_p (exp))
+ if (TREE_SIDE_EFFECTS (exp) || generic_expr_could_trap_p (exp))
return false;
while (CONVERT_EXPR_P (exp))
--- gcc/tree-ssa-sccvn.c.jj 2019-09-24 14:39:07.479465423 +0200
+++ gcc/tree-ssa-sccvn.c2019-10-11 13:21:47.437326054 +0200
@@ -5100,18 +5100,15 @@ vn_nary_may_trap (vn_nary_op_t nary)
honor_nans = flag_trapping_math && !flag_finite_math_only;
honor_snans = flag_signaling_nans != 0;
}
- else if (INTEGRAL_TYPE_P (type)
- && TYPE_OVERFLOW_TRAPS (type))
+ else if (INTEGRAL_TYPE_P (type) && TYPE_OVERFLOW_TRAPS (type))
honor_trapv = true;
}
if (nary->length >= 2)
rhs2 = nary->op[1];
ret = operation_could_trap_helper_p (nary->opcode, fp_operation,
- honor_trapv,
- honor_nans, honor_snans, rhs2,
- &handled);
- if (handled
- && ret)
+ honor_trapv, honor_nans, honor_snans,
+ rhs2, &handled);
+ if (handled && ret)
return true;
for (i = 0; i < nary->length; ++i)
--- gcc/testsuite/gcc.c-torture/compile/pr92063.c.jj2019-10-11
11:51:47.959149238 +0200
+++
Re: [PATCH] rs6000: -flto forgets 'no-vsx' function attributes (PR target/70010)
Hi! On Sat, Oct 12, 2019 at 11:22:15AM +0800, Jiufu Guo wrote: > As expected in the PR, when a function is marked with no-vsx, we would > assume user has good reason to disable VSX code generation for the function. > To avoid VSX code generation, this function should not be inlined into VSX > function. But your patch also disables inlining if the callee merely defaulted to no-vsx. Can you see if the option is explicitly disabled here? > PR target/70010 > * config/rs6000/rs6000.c (rs6000_can_inline_p): Check 'vsx' feature > for caller and callee Each sentence should end with a period, also in changelogs. > PR target/70010 > * gcc.target/powerpc/inline_error.c: New test Ditto. > - /* Callee's options should a subset of the caller's, i.e. a vsx > function > - can inline an altivec function but a non-vsx function can't inline a > - vsx function. */ > - if ((caller_opts->x_rs6000_isa_flags & callee_opts->x_rs6000_isa_flags) > - == callee_opts->x_rs6000_isa_flags) > + /* Callee's options should a subset of the caller's. In addition, > + vsx function should not inline function with non-vsx by which > + programmer may intend to disable VSX code generation. */ Two spaces after a period. How about something like /* Callee's options should be a subset of the caller's. Also, a function without VSX enabled should not be inlined into one with VSX enabled, because it may be important it is disabled there; see PR70010. */ It's not clear to me why this is important, and what makes -mvsx different from all other similar options? Segher
Re: [PATCH target/92035] Add missing avx512f intrinsics
On Sat, Oct 12, 2019 at 4:15 PM Jakub Jelinek wrote:
>
> Hi!
>
> > gcc/
> > * config/i386/avx512fintrin.h (_mm_mask_roundscale_ss,
> > _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> > _mm_maskz_roundscale_round_ss, _mm_mask_roundscale_sd,
> > _mm_maskz_roundscale_sd, _mm_mask_roundscale_round_sd,
> > _mm_maskz_roundscale_round_sd): New intrinsics.
> > (_mm_roundscale_ss, _mm_roundscale_round_ss): Fix.
>
> "Fix." doesn't describe the change you've done. So I think it should be
> instead:
> "Use __builtin_ia32_rndscales?_mask_round builtins instead of
> __builtin_ia32_rndscales?_round."
>
> > * config/i386/i386-builtin.def (__builtin_ia32_rndscaless_round,
> > __builtin_ia32_rndscalesd_round): Remove.
> > (__builtin_ia32_rndscalesd_mask_round,
>
> Pasto, sd listed twice, ss not listed, change the first one to ss.
>
> > __builtin_ia32_rndscalesd_mask_round): New intrinsics.
> > * config/i386/sse.md (avx512f_rndscale):
> > Renamed to ...
> >
> > (avx512f_rndscale): ...
> > this.
>
> These two lines are too long. Perhaps:
> * config/i386/sse.md
> (avx512f_rndscale): Renamed to ...
> (avx512f_rndscale):
> ... this.
>
> > ((match_operand:VF_128 2 ""
> > "")): Changed to ...
> > ((match_operand:VF_128 2 ""
> > "")): ... this.
> > ("vrndscale\t{%3, %2, %1,
> > %0|%0, %1, %2, %3}"): Changed to ...
> >
> > ("vrndscale\t{%3,%2, %1,
> > %0|%0, %1,
> > %2, %3}"): ... this.
>
> But the above is not appropriate, the ChangeLog in *.md is at the level of
> define_{insn,expand,split,peephole2,insn_and_split} etc., not at the level
> of individual subrtls or patterns.
> So, the right thing is just to ammend the "... this.", follow it up by a
> sentence what also changed. Like " Adjust and add subst attributes to make
> it maskable."
>
> >
> > gcc/testsuite/
> > * gcc.target/i386/avx512f-vrndscaless-1.c (_mm_mask_roundscale_ss,
> > _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> > _mm_maskz_roundscale_round_ss): Test new intrinsics.
>
> That is again not what you've changed. For tests, often the exact
> spots aren't listed and one just uses *.c: Description. but if you want to
> use details, you can e.g.
> * gcc.target/i386/avx512f-vrndscaless-1.c: Add scan-assembler-times
> directives for newly expected instructions.
> (m): New variable.
> (avx512f_test): Add tests for new intrinsics.
>
> > * gcc.target/i386/avx512f-vrndscaless-2.c (_mm_mask_roundscale_ss,
> > _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> > _mm_maskz_roundscale_round_ss): Test new intrinsics.
> > * gcc.target/i386/avx512f-vrndscalesd-1.c (_mm_mask_roundscale_sd,
> > _mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
> > _mm_maskz_roundscale_round_sd): Test new intrinsics.
> > * gcc.target/i386/avx512f-vrndscalesd-2.c (_mm_mask_roundscale_sd,
> > _mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
> > _mm_maskz_roundscale_round_sd): Test new intrinsics.
>
> Likewise.
>
> > * gcc.target/i386/avx-1.c (__builtin_ia32_rndscalefss_round,
> > __builtin_ia32_rndscalefsd_round): Remove builtin.
> > (__builtin_ia32_rndscalefss_mask_round,
> > __builtin_ia32_rndscalefsd_mask_round): Test new builtin.
>
> That is not what you've changed. You are there not Removing a builtin
> and testing a new builtin, but removing a macro and defining a new macro.
> So I think
> : Remove.
> : Define.
> is more appropriate.
>
> > +#define _mm_roundscale_round_ss(A, B, I, R)
> > \
> > + ((__m128) __builtin_ia32_rndscaless_mask_round ((__v4sf)(__m128)(A),
> > \
> > + (__v4sf)(__m128)(B), \
> > + (int)(I), \
> > + (__v4sf)_mm_setzero_ps(),\
>
> There should be a space after _mm_setzero_p[sd] .
> I know the formatting of many macros is just wrong, but no need to add more
> of that.
>
> Ok for trunk with those nits fixed.
>
> In fact, it would be probably cleaner to:
> ((__m128) \
>__builtin_ia32_rndscaless_mask_round ((__v4sf) (__m128) (A), \
> (__v4sf) (__m128) (B), \
> (int) (I), \
> (__v4sf) _mm_setzero_ps (),\
> (__mmask8) (-1), \
> (int) (R))
> or so, because then one has for long builtin names more space. But
> I'm not asking for that to be changed.
>
> Jakub
Thanks for your review, that helps a lot.
--
BR,
Hongtao
Re: [PATCH][AArch64] Fix symbol offset limit
Wilco Dijkstra writes:
> Hi Richard,
>
>> If global_char really is a char then isn't that UB?
>
> No why?
Well, "simple expressions like &global_char + 0xff00" made it sound
like there really was a:
extern char global_char;
Only &global_char and &global_char + 1 are defined in that case.
I was probably taking the example too literally though.
> We can do all kinds of arithmetic based on pointers, either using
> pointer types or converted to uintptr_t. Note that the optimizer
> actually creates these expressions, for example arr[N-x] can be
> evaluated as (&arr[0] + N) - x. So this remains legal even if N is
> well outside the bounds of the array.
Yeah, true, and I remember valid "x[n - big_offset]" -> invalid
"x + big_offset - n" being a problem for MIPS too. It was one of
the things offset_within_block_p was helping to avoid.
>> I guess we still have to compile it without error though...
>
> *THIS* a million times... Any non-trivial application relies on UB behaviour
> with
> 100% certainty. But we still have to compile it correctly!
>
>>> To avoid this, limit the offset to +/-1MB so that the symbol needs to
>>> be within a
>>> 3.9GB offset from its references. For the tiny code model use a 64KB
>>> offset, allowing
>>> most of the 1MB range for code/data between the symbol and its
>>> references.
>>
>> These new values seem a bit magical. Would it work for the original
>> testcase to use FORCE_TO_MEM if !offset_within_block_p (x, offset)?
>
> No - the testcases fail with that.
Hmm, OK. Could you give more details? What does the motivating case
actually look like?
One of the reasons this is a hard patch to review is that it doesn't
include a testcase for the kind of situation it's trying to fix.
> It also reduces codequality by not allowing commonly used offsets as
> part of the symbol relocation.
Which kinds of cases specifically? Although I'm guessing the problem is
when indexing into external arrays of unknown size.
I think the starting point for this stuff is that the address of every
referenced object has to be within reach for the code model. In other
words, offset 0 must be within reach. It follows that the end + 1
address of every referenced object except the last (in memory) will
also be within reach.
The question then is: do we want, as a special exception, to allow
the end + 1 address of the last object to be out of reach? I think
it would be very difficult to support that reliably without more
information. The compiler would be left guessing which object is
the final one and how much of it is out of reach.
So IMO we should be able to assume that the start and end + 1 addresses
of any referenced object are within reach. For section anchors, we can
extend that to the containing anchor block, which is really just a
super-sized object.
So when offset_within_block_p, it seems like we should apply the
documented ranges for the code model, or even ignore them altogether.
Limiting the range to 64k for the tiny model would regress valid
cases like:
char x[0x2];
char f (void) { return x[0x1]; }
I'm not saying this case is important. I'm just saying there's no
obvious reason why the offset shouldn't be valid given the above
assumptions.
The question then is what to do about symbols whose size isn't known.
And I agree that unconditionally applying the full code-model offset
range is too aggressive in that case.
> So how would you want to make the offsets less magical? There isn't a clear
> limitation
> like for MOVW/MOVT relocations (which are limited to +-32KB), so it just is
> an arbitrary
> decision which allows this optimization for all frequently used offsets but
> avoids relocation
> failures caused by large offsets. The values I used were chosen so that
> codequality isn't
> affected, but it is practically impossible to trigger relocation errors.
>
> And they can always be changed again if required - this is not meant to be
> the final
> AArch64 patch ever!
Well, for one thing, if code quality isn't affected by using +/-64k
for the tiny model, do we have any evidence that we need a larger offset
for the other code models?
But more importantly, we can't say definitively that code quality isn't
affected, only that it wasn't affected for the cases we've looked at.
People could patch the compiler if the new ranges turn out not to strike
the right balance for their use cases, but not everyone wants to do that.
Maybe we need a new command-line option. It seems like the cheap
way out, but it might be justified. The problem is that, without one,
the compiler would be unconditionally applying an offset range that has
no obvious relation (from a user's POV) to the documented -mcmodel range.
We can then set the option's default value to whatever we think is
reasonable. And FWIW I've no specific issue with the values you've
chosen except for the question above.
Thanks,
Richard
Re: [PATCH][AArch64] Fix symbol offset limit
Richard Sandiford writes: > Wilco Dijkstra writes: >> We can do all kinds of arithmetic based on pointers, either using >> pointer types or converted to uintptr_t. Note that the optimizer >> actually creates these expressions, for example arr[N-x] can be >> evaluated as (&arr[0] + N) - x. So this remains legal even if N is >> well outside the bounds of the array. > > Yeah, true, and I remember valid "x[n - big_offset]" -> invalid > "x + big_offset - n" being a problem for MIPS too. It was one of > the things offset_within_block_p was helping to avoid. Doh, I of course meant "x - big_offset + n" here ;-)
RE: [PATCH 1/2][GCC][RFC][middle-end]: Add SLP pattern matcher.
Hi Richi, > -Original Message- > From: Richard Biener > Sent: Friday, October 11, 2019 8:02 AM > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; nd ; o...@ucw.cz > Subject: Re: [PATCH 1/2][GCC][RFC][middle-end]: Add SLP pattern matcher. > > On Tue, 8 Oct 2019, Tamar Christina wrote: > > > Hi Richi, > > > > Thanks for the review, I've added some comments inline. > > > > The 10/07/2019 12:15, Richard Biener wrote: > > > On Tue, 1 Oct 2019, Tamar Christina wrote: > > > > > > > Hi All, > > > > > > > > This adds a framework to allow pattern matchers to be written at > > > > based on the SLP tree. The difference between this one and the > > > > one in tree-vect-patterns is that this matcher allows the matching > > > > of an arbitrary number of parallel statements and replacing of an > arbitrary number of children or statements. > > > > > > > > Any relationship created by the SLP pattern matcher will be undone if > SLP fails. > > > > > > > > The pattern matcher can also cancel all permutes depending on what > > > > the pattern requested it to do. As soon as one pattern requests > > > > the permutes to be cancelled all permutes are cancelled. > > > > > > > > Compared to the previous pattern matcher this one will work for an > > > > arbitrary group size and will match at any arbitrary node in the > > > > SLP tree. The only requirement is that the entire node is matched or > rejected. > > > > > > > > vect_build_slp_tree_1 is a bit more lenient in what it accepts as > > > > "compatible operations" but the matcher cannot be because in cases > > > > where you match the order of the operands may be changed. So all > operands must be changed or none. > > > > > > > > Furthermore the matcher relies on canonization of the operations > > > > inside the SLP tree and on the fact that floating math operations are > > > > not > commutative. > > > > This means that matching a pattern does not need to try all > > > > alternatives or combinations and that arguments will always be in > > > > the same order if it's the same operation. > > > > > > > > The pattern matcher also ignored uninteresting nodes such as type > > > > casts, loads and stores. Doing so is essential to keep the runtime > > > > down. > > > > > > > > Each matcher is allowed a post condition that can be run to > > > > perform any changes to the SLP tree as needed before the patterns > > > > are created and may also abort the creation of the patterns. > > > > > > > > When a pattern is matched it is not immediately created but > > > > instead it is deferred until all statements in the node have been > > > > analyzed. Only if all post conditions are true, and all > > > > statements will be replaced will the patterns be created in batch. > > > > This allows us to not have to undo any work if the pattern fails but > > > > also > makes it so we traverse the tree only once. > > > > > > > > When a new pattern is created it is a marked as a pattern to the > > > > statement it is replacing and be marked as used in the current SLP > > > > scope. If SLP fails then relationship is undone and the relevancy > restored. > > > > > > > > Each pattern matcher can detect any number of pattern it wants. > > > > The only constraint is that the optabs they produce must all have the > same arity. > > > > > > > > The pattern matcher supports instructions that have no scalar form > > > > as they are added as pattern statements to the stmt. The BB is > > > > left untouched and so the scalar loop is untouched. > > > > > > > > Bootstrapped on aarch64-none-linux-gnu and no issues. > > > > No regression testing done yet. > > > > > > If you split out the introduction of SLP_TREE_REF_COUNT you can > > > commit that right now (sorry for being too lazy there...). > > > > > > > I'll split those off :) > > > > > One overall comment - you do pattern matching after SLP tree > > > creation (good) but still do it before the whole SLP graph is > > > created (bad). Would it be possible to instead do it as a separate > > > phase in vect_analyze_slp, looping over all instances (the instances > > > present entries into the single unified SLP graph now), avoiding to > > > visit "duplicates"? > > > > > > > It should be, the only issue I can see is that build SLP may fail > > because of an unsupported permute, or because it can use load lanes. > > If I'm understanding it correctly you wouldn't get SLP vectorization > > in those cases so then the matching can't work? So it would limit it a it > > more. > > True. As said later ideally the pattern matching woudl happen before or > rather as part of SLP building so that the operations then actually match (and > we then don't need that plus/minus SLP_TREE_TWO_OPERATORS handling). > At the moment it sits in a quite awkward place which may contribute to its > complexity. > > > > In patch 1/2 I see references (mostly in variable names) to > > > "complex" which is IMHO too specific. > > > > > > > Sorry, missed those during my cleanup. > > > >
Re: [PATCH] Fix up *COND_EXPR *trap* handling (PR middle-end/92063)
On October 12, 2019 10:44:06 AM GMT+02:00, Jakub Jelinek
wrote:
>Hi!
>
>As mentioned in the PR and on IRC, tree_could_trap_p is described
>as taking GIMPLE expressions only, but in fact we rely on it not
>crashing
>when feeded GENERIC, just for GENERIC it will not handle expressions
>recursively and we have generic_expr_could_trap_p for that that calls
>tree_could_trap_p recursively.
>The addition of != COND_EXPR and != VEC_COND_EXPR assert to
>operation_could_trap_p broke this, because if GENERIC with COND_EXPR
>being first argument (condition) of another COND_EXPR is fed to
>tree_could_trap_p, it now ICEs.
>
>The following patch fixes it by:
>1) in tree_could_trap_p return false for {,VEC_}COND_EXPR rather than
>just recursing on the first argument. For GIMPLE, we shouldn't be
>called
>with {,VEC_}COND_EXPR, because those are ternary rhs and thus 3
>arguments,
>not one. For GENERIC, we can, but we also need to recurse and the
>recursion
>should discover that the comparison may trap.
>2) in simple_operand_p_2 which calls tree_could_trap_p on GENERIC, this
>is
>changed to generic_expr_could_trap_p so that it actually recurses and
>tests
>subexpressions too
>3) in operation_could_trap_helper_p signals that *COND_EXPR is not
>handled
>and it doesn't care about whether *COND_EXPR is floating point or not.
>This change is for sccvn, which calls operation_could_trap_helper_p
>and then, if not handled, calls tree_could_trap_p on the operands.
>Without the first hunk, *COND_EXPR is handled by the default handling,
>which says that fp_operation can trap (but *COND_EXPR with fp operands
>can't
>in itself) and makes it unhandled otherwise, at which point we call
>tree_could_trap_p on the condition, which is all that matters.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
>2019-10-12 Jakub Jelinek
>
> PR middle-end/92063
> * tree-eh.c (operation_could_trap_helper_p)
> : Return false with *handled = false.
> (tree_could_trap_p): For {,VEC_}COND_EXPR return false instead of
> recursing on the first operand.
> * fold-const.c (simple_operand_p_2): Use generic_expr_could_trap_p
> instead of tree_could_trap_p.
> * tree-ssa-sccvn.c (vn_nary_may_trap): Formatting fixes.
>
> * gcc.c-torture/compile/pr92063.c: New test.
>
>--- gcc/tree-eh.c.jj 2019-10-07 17:30:31.028153702 +0200
>+++ gcc/tree-eh.c 2019-10-11 13:46:12.626811039 +0200
>@@ -2499,6 +2499,14 @@ operation_could_trap_helper_p (enum tree
> /* Constructing an object cannot trap. */
> return false;
>
>+case COND_EXPR:
>+case VEC_COND_EXPR:
>+ /* Whether *COND_EXPR can trap depends on whether the
>+ first argument can trap, so signal it as not handled.
>+ Whether lhs is floating or not doesn't matter. */
>+ *handled = false;
>+ return false;
>+
> default:
> /* Any floating arithmetic may trap. */
> if (fp_operation && flag_trapping_math)
>@@ -2614,9 +2622,12 @@ tree_could_trap_p (tree expr)
> if (!expr)
> return false;
>
>- /* For COND_EXPR and VEC_COND_EXPR only the condition may trap. */
>+ /* In COND_EXPR and VEC_COND_EXPR only the condition may trap, but
>+ they won't appear as operands in GIMPLE form, so this is just for
>the
>+ GENERIC uses where it needs to recurse on the operands and so
>+ *COND_EXPR itself doesn't trap. */
>if (TREE_CODE (expr) == COND_EXPR || TREE_CODE (expr) == VEC_COND_EXPR)
>-expr = TREE_OPERAND (expr, 0);
>+return false;
>
> code = TREE_CODE (expr);
> t = TREE_TYPE (expr);
>--- gcc/fold-const.c.jj2019-10-09 10:27:12.578402783 +0200
>+++ gcc/fold-const.c 2019-10-11 12:29:53.426603712 +0200
>@@ -4447,8 +4447,7 @@ simple_operand_p_2 (tree exp)
> {
> enum tree_code code;
>
>- if (TREE_SIDE_EFFECTS (exp)
>- || tree_could_trap_p (exp))
>+ if (TREE_SIDE_EFFECTS (exp) || generic_expr_could_trap_p (exp))
> return false;
>
> while (CONVERT_EXPR_P (exp))
>--- gcc/tree-ssa-sccvn.c.jj2019-09-24 14:39:07.479465423 +0200
>+++ gcc/tree-ssa-sccvn.c 2019-10-11 13:21:47.437326054 +0200
>@@ -5100,18 +5100,15 @@ vn_nary_may_trap (vn_nary_op_t nary)
> honor_nans = flag_trapping_math && !flag_finite_math_only;
> honor_snans = flag_signaling_nans != 0;
> }
>- else if (INTEGRAL_TYPE_P (type)
>- && TYPE_OVERFLOW_TRAPS (type))
>+ else if (INTEGRAL_TYPE_P (type) && TYPE_OVERFLOW_TRAPS (type))
> honor_trapv = true;
> }
> if (nary->length >= 2)
> rhs2 = nary->op[1];
> ret = operation_could_trap_helper_p (nary->opcode, fp_operation,
>- honor_trapv,
>- honor_nans, honor_snans, rhs2,
>- &handled);
>- if (handled
>- && ret)
>+ honor_trapv, honor_nans, honor_snans,
>+ rhs2, &
Re: [PATCH] PR fortran/90297 -- Remove code with no functional effect
Hi Steve, As I remarked on the PR thread, it is one of the less harmful bits of code that I introduced :-) OK to commit. Thanks Paul On Sat, 12 Oct 2019 at 01:17, Steve Kargl wrote: > > The patch is fairly self-explanatory. OK to commit? > > 2019-10-11 Steven G. Kargl > > PR fortran/90297 > * resolve.c (resolve_typebound_function): Remove code with no > functional effect. > > -- > Steve -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
Re: [PATCH] PR fortran/89943 -- Duplicate BIND(c) allowed (?)
Hi Steve,
In the F2018 standard: C1550 (R1526) If MODULE appears in the prefix
of a module subprogram and a binding label is specified, it
shall be the same as the binding label specified in the corresponding
module procedure interface body.
While it does not say explicitly that a repeat binding label is
allowed, I think that the implication is clear enough.
The patch is OK as it is but it would be nice if C1550 is or would be
implemented.
Thanks
Paul
On Fri, 11 Oct 2019 at 19:31, Steve Kargl
wrote:
>
> PING.
>
> On Fri, Oct 04, 2019 at 03:26:53PM -0700, Steve Kargl wrote:
> > The attached patch allows the declaration of a BIND(C)
> > module function or module subroutine to appear in a
> > submodule (see testcases). Regression test was clean.
> > OK to commit?
> >
> > Before a rubber stamped 'OK'. I do NOT use submodules.
> > A submodule user needs to pipe up on the validity of
> > the patch.
> >
> >
> > 2019-10-04 Steven G. Kargl
> >
> > PR fortran/89943
> > decl.c (gfc_match_function_decl): Ignore duplicate BIND(C) for
> > function
> > declaration in submodule.
> > (gfc_match_entry): Use temporary for locus, which allows removal of
> > one gfc_error_now().
> > (gfc_match_subroutine): Ignore duplicate BIND(C) for subroutine
> > declaration in submodule.
> >
> > 2019-10-04 Steven G. Kargl
> >
> > PR fortran/89943
> > * gfortran.dg/pr89943_1.f90: New test.
> > * gfortran.dg/pr89943_2.f90: Ditto.
> >
> > --
> > Steve
>
> > Index: gcc/fortran/decl.c
> > ===
> > --- gcc/fortran/decl.c(revision 276601)
> > +++ gcc/fortran/decl.c(working copy)
> > @@ -7259,13 +7259,16 @@ gfc_match_function_decl (void)
> >if (sym->attr.is_bind_c == 1)
> > {
> >sym->attr.is_bind_c = 0;
> > - if (sym->old_symbol != NULL)
> > -gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > - "variables or common blocks",
> > - &(sym->old_symbol->declared_at));
> > - else
> > -gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > - "variables or common blocks", &gfc_current_locus);
> > +
> > + if (gfc_state_stack->previous
> > + && gfc_state_stack->previous->state != COMP_SUBMODULE)
> > + {
> > + locus loc;
> > + loc = sym->old_symbol != NULL
> > + ? sym->old_symbol->declared_at : gfc_current_locus;
> > + gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > + "variables or common blocks", &loc);
> > + }
> > }
> >
> >if (found_match != MATCH_YES)
> > @@ -7517,16 +7520,16 @@ gfc_match_entry (void)
> > not allowed for procedures. */
> >if (entry->attr.is_bind_c == 1)
> > {
> > + locus loc;
> > +
> >entry->attr.is_bind_c = 0;
> > - if (entry->old_symbol != NULL)
> > -gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > - "variables or common blocks",
> > - &(entry->old_symbol->declared_at));
> > - else
> > -gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > - "variables or common blocks", &gfc_current_locus);
> > -}
> >
> > + loc = entry->old_symbol != NULL
> > + ? entry->old_symbol->declared_at : gfc_current_locus;
> > + gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > + "variables or common blocks", &loc);
> > + }
> > +
> >/* Check what next non-whitespace character is so we can tell if there
> > is the required parens if we have a BIND(C). */
> >old_loc = gfc_current_locus;
> > @@ -7725,13 +7728,16 @@ gfc_match_subroutine (void)
> >if (sym->attr.is_bind_c == 1)
> > {
> >sym->attr.is_bind_c = 0;
> > - if (sym->old_symbol != NULL)
> > -gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > - "variables or common blocks",
> > - &(sym->old_symbol->declared_at));
> > - else
> > -gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > - "variables or common blocks", &gfc_current_locus);
> > +
> > + if (gfc_state_stack->previous
> > + && gfc_state_stack->previous->state != COMP_SUBMODULE)
> > + {
> > + locus loc;
> > + loc = sym->old_symbol != NULL
> > + ? sym->old_symbol->declared_at : gfc_current_locus;
> > + gfc_error_now ("BIND(C) attribute at %L can only be used for "
> > + "variables or common blocks", &loc);
> > + }
> > }
> >
> >/* C binding names are not allowed for internal procedures. */
> > Index: gcc/testsuite/gfortran.dg/pr89943_1.f90
> > ===
> > --- gcc/testsuite/gfortran.dg/pr89
Re: [PATCH 2/2] Documentation hyperlinks for [-Wname-of-option] (PR 87488)
On Sat, 2019-10-12 at 07:00 +, Bernd Edlinger wrote:
> Hi David,
>
> I just wanted to say that this does not work right on ubuntu 14.04 at
> least:
>
> g++ -Wshadow=local -Wno-shadow=compatible-local -S Wshadow-1.c
> Wshadow-1.c: In function 'void foo(int*, int*)':
> Wshadow-1.c:8:9: warning: declaration of 'int d' shadows a parameter
> []8;;
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local-Wshadow=local]8
> ;;]
> 8 | int d = 0; /* { dg-warning "Wshadow=local" }
> */
> | ^
> Wshadow-1.c:4:23: note: shadowed declaration is here
> 4 | void foo(int *c, int *d) /* { dg-bogus "Wshadow" } */
> | ~^
> Wshadow-1.c: In function 'void bar(int*, int*)':
> Wshadow-1.c:15:15: warning: declaration of 'c' shadows a global
> declaration []8;;
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8
> ;;]
>15 | void bar(int *c, int *d) /* { dg-warning "Wshadow" } */
> | ~^
> Wshadow-1.c:3:5: note: shadowed declaration is here
> 3 | int c;
> | ^
> Wshadow-1.c:19:9: warning: declaration of 'int d' shadows a parameter
> []8;;
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8
> ;;]
>19 | int d = 0; /* { dg-warning "Wshadow" } */
> | ^
> Wshadow-1.c:15:23: note: shadowed declaration is here
>15 | void bar(int *c, int *d) /* { dg-warning "Wshadow" } */
> | ~^
> Wshadow-1.c:22:10: warning: declaration of 'e' shadows a previous
> local []8;;
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8
> ;;]
>22 | int *e = 0;/* { dg-warning "Wshadow" } */
> | ^
> Wshadow-1.c:17:8: note: shadowed declaration is here
>17 | int *e = d;
> |^
>
> Coloring works, even the hyperlinks seem to work, when clicked at,
> although they jump just
> to the top of the page, and one has to scroll down to the warning
> manually.
> But the warning name is very hard to spot in all that glibberish long
> text :-(
Thanks for the report.
Bother.
There are (at least) two issues here.
(a) It sounds like your terminal doesn't handle the escape sequence my
patch is emitting. The expected outcome is that the terminal embeds
this URL:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local
within:
[-Wshadow=local]
i.e. equivalent to:
[https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local";
;;]
It seems like instead it's displaying this:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local-Wshadow=local%1B]8
When you say "even the hyperlinks seem to work" it seems like that URL
fragment is being handled by the terminal's regular "look for URLs in
the text" logic.
Does your terminal have a "Copy URL" option (perhaps on a right-click
menu)? If so, what URL is copied?
Which terminal (and version) are you seeing this with? (if gnome
terminal, which vte version?)
What happens if you try this at the terminal:
printf '\e]8;;http://example.com\e\\This is a link\e]8;;\e\\\n'
(hopefully that doesn't get mangled by my mailer; this is the example
from:
https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda
)
I don't yet know if there's a way to query the terminal to see if the
escape is supported. My hope was that they would be silently
discarded.
> Do you see a way to find out if the escape sequences are supported,
> or how to disable this function per configure option?
Do you mean a GCC configure-time option to change the default? We have
that for colors; we could add one for URLs. (Though I'd prefer to know
more about the scope of the problem first)
The other problem:
(b) The URL itself is wrong; for this option it ought to be:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow
(i.e. without the "=local"). As noted by Manu it sounds like the
implementation needs to be smarter about generating URLs.
Thanks again for the report.
Is this working/broken for other people?
Thanks
Dave
[Ada] Fix PR ada/91995
The function Defining_Entity recently gained new parameters with default values, namely Empty_On_Errors and Concurrent_Subunit. It turns out that these new parameters are not really necessary and can be easily removed, which will also save a few bytes in the final binary. Tested on x86_64-suse-linux, applied on all active branches. 2019-10-12 Eric Botcazou PR ada/91995 * sem_ch8.adb (Chain_Use_Clause): Remove second argument in calls to Defining_Entity. * sem_elab.adb (Find_Unit_Entity): Likewise. Deal with N_Subunit here in lieu of in Defining_Entity. * sem_util.ads (Defining_Entity): Remove 2nd and 3th parameters. * sem_util.adb (Defining_Entity): Remove 2nd and 3th parameters, and adjust accordingly. Deal with N_Compilation_Unit. -- Eric BotcazouIndex: sem_ch8.adb === --- sem_ch8.adb (revision 276875) +++ sem_ch8.adb (working copy) @@ -4290,16 +4290,14 @@ package body Sem_Ch8 is -- Common case for compilation unit - elsif Defining_Entity (N => Parent (N), - Empty_On_Errors => True) = Current_Scope - then + elsif Defining_Entity (Parent (N)) = Current_Scope then null; else -- If declaration appears in some other scope, it must be in some -- parent unit when compiling a child. - Pack := Defining_Entity (Parent (N), Empty_On_Errors => True); + Pack := Defining_Entity (Parent (N)); if not In_Open_Scopes (Pack) then null; Index: sem_elab.adb === --- sem_elab.adb (revision 276875) +++ sem_elab.adb (working copy) @@ -9103,13 +9103,23 @@ package body Sem_Elab is N_Procedure_Instantiation) and then Nkind (Context) = N_Compilation_Unit then - return - Related_Instance (Defining_Entity (N, Concurrent_Subunit => True)); + return Related_Instance (Defining_Entity (N)); + + -- The unit denotes a concurrent body acting as a subunit. Such bodies + -- are generally rewritten into null statements. The proper entity is + -- that of the "original node". + + elsif Nkind (N) = N_Subunit +and then Nkind (Proper_Body (N)) = N_Null_Statement +and then Nkind_In (Original_Node (Proper_Body (N)), N_Protected_Body, +N_Task_Body) + then + return Defining_Entity (Original_Node (Proper_Body (N))); -- Otherwise the proper entity is the defining entity else - return Defining_Entity (N, Concurrent_Subunit => True); + return Defining_Entity (N); end if; end Find_Unit_Entity; Index: sem_util.adb === --- sem_util.adb (revision 276875) +++ sem_util.adb (working copy) @@ -5867,11 +5867,7 @@ package body Sem_Util is -- Defining_Entity -- - - function Defining_Entity - (N : Node_Id; - Empty_On_Errors: Boolean := False; - Concurrent_Subunit : Boolean := False) return Entity_Id - is + function Defining_Entity (N : Node_Id) return Entity_Id is begin case Nkind (N) is when N_Abstract_Subprogram_Declaration @@ -5922,24 +5918,11 @@ package body Sem_Util is => return Defining_Identifier (N); - when N_Subunit => -declare - Bod : constant Node_Id := Proper_Body (N); - Orig_Bod : constant Node_Id := Original_Node (Bod); - -begin - -- Retrieve the entity of the original protected or task body - -- if requested by the caller. + when N_Compilation_Unit => +return Defining_Entity (Unit (N)); - if Concurrent_Subunit - and then Nkind (Bod) = N_Null_Statement - and then Nkind_In (Orig_Bod, N_Protected_Body, N_Task_Body) - then - return Defining_Entity (Orig_Bod); - else - return Defining_Entity (Bod); - end if; -end; + when N_Subunit => +return Defining_Entity (Proper_Body (N)); when N_Function_Instantiation | N_Function_Specification @@ -5965,14 +5948,10 @@ package body Sem_Util is -- can continue semantic analysis. elsif Nam = Error then - if Empty_On_Errors then - return Empty; - else - Err := Make_Temporary (Sloc (N), 'T'); - Set_Defining_Unit_Name (N, Err); + Err := Make_Temporary (Sloc (N), 'T'); + Set_Defining_Unit_Name (N, Err); -
Re: [PATCH 2/2] Documentation hyperlinks for [-Wname-of-option] (PR 87488)
On 10/12/19 4:21 PM, David Malcolm wrote:
> On Sat, 2019-10-12 at 07:00 +, Bernd Edlinger wrote:
>> Hi David,
>>
>> I just wanted to say that this does not work right on ubuntu 14.04 at
>> least:
>>
>> g++ -Wshadow=local -Wno-shadow=compatible-local -S Wshadow-1.c
>> Wshadow-1.c: In function 'void foo(int*, int*)':
>> Wshadow-1.c:8:9: warning: declaration of 'int d' shadows a parameter
>> []8;;
>> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local-Wshadow=local]8
>> ;;]
>> 8 | int d = 0; /* { dg-warning "Wshadow=local" }
>> */
>> | ^
>> Wshadow-1.c:4:23: note: shadowed declaration is here
>> 4 | void foo(int *c, int *d) /* { dg-bogus "Wshadow" } */
>> | ~^
>> Wshadow-1.c: In function 'void bar(int*, int*)':
>> Wshadow-1.c:15:15: warning: declaration of 'c' shadows a global
>> declaration []8;;
>> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8
>> ;;]
>>15 | void bar(int *c, int *d) /* { dg-warning "Wshadow" } */
>> | ~^
>> Wshadow-1.c:3:5: note: shadowed declaration is here
>> 3 | int c;
>> | ^
>> Wshadow-1.c:19:9: warning: declaration of 'int d' shadows a parameter
>> []8;;
>> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8
>> ;;]
>>19 | int d = 0; /* { dg-warning "Wshadow" } */
>> | ^
>> Wshadow-1.c:15:23: note: shadowed declaration is here
>>15 | void bar(int *c, int *d) /* { dg-warning "Wshadow" } */
>> | ~^
>> Wshadow-1.c:22:10: warning: declaration of 'e' shadows a previous
>> local []8;;
>> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow]8
>> ;;]
>>22 | int *e = 0;/* { dg-warning "Wshadow" } */
>> | ^
>> Wshadow-1.c:17:8: note: shadowed declaration is here
>>17 | int *e = d;
>> |^
>>
>> Coloring works, even the hyperlinks seem to work, when clicked at,
>> although they jump just
>> to the top of the page, and one has to scroll down to the warning
>> manually.
>> But the warning name is very hard to spot in all that glibberish long
>> text :-(
>
> Thanks for the report.
>
> Bother.
>
> There are (at least) two issues here.
>
> (a) It sounds like your terminal doesn't handle the escape sequence my
> patch is emitting. The expected outcome is that the terminal embeds
> this URL:
>
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local
>
> within:
>
> [-Wshadow=local]
>
> i.e. equivalent to:
>
> [https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local";
> ;;]
>
> It seems like instead it's displaying this:
>
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local-Wshadow=local%1B]8
>
> When you say "even the hyperlinks seem to work" it seems like that URL
> fragment is being handled by the terminal's regular "look for URLs in
> the text" logic.
>
> Does your terminal have a "Copy URL" option (perhaps on a right-click
> menu)? If so, what URL is copied?
>
Yes, the URL is
"https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow-Wshadow";
> Which terminal (and version) are you seeing this with? (if gnome
> terminal, which vte version?)
>
I think the process that hosts the "Terminal" is
/usr/bin/xfce4-terminal
The a Help/About menu says:
xfce4-terminal 0.6.3
> What happens if you try this at the terminal:
>
> printf '\e]8;;http://example.com\e\\This is a link\e]8;;\e\\\n'
>
I see this:
]8;;http://example.comThis is a link]8;;
the ] is actually a little square with little 0101 in it.
again the URL is copied. This time it is "http://example.comThis";
> (hopefully that doesn't get mangled by my mailer; this is the example
> from:
> https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda
> )
>
Both show the same info.
> I don't yet know if there's a way to query the terminal to see if the
> escape is supported. My hope was that they would be silently
> discarded.
>
I wonder why this is not done with a ncurses function?
In case it helps this is the environment I have:
$ export
declare -x CLUTTER_IM_MODULE="xim"
declare -x COLORTERM="xfce4-terminal"
declare -x DBUS_SESSION_BUS_ADDRESS="unix:abstract=/tmp/dbus-Wh3VZkTGvf"
declare -x DEFAULTS_PATH="/usr/share/gconf/ubuntustudio.default.path"
declare -x DESKTOP_SESSION="ubuntustudio"
declare -x DISPLAY=":0.0"
declare -x GDMSESSION="ubuntustudio"
declare -x GDM_LANG="de_DE"
declare -x GLADE_CATALOG_PATH=":"
declare -x GLADE_MODULE_PATH=":"
declare -x GLADE_PIXMAP_PATH=":"
declare -x GNOME_KEYRING_CONTROL="/run/user/1000/keyring-V870cQ"
declare -x GNOME_KEYRING_PID="1770"
declare -x GPG_AGENT_INFO="/run/user/1000/keyring-V870cQ/gpg:0:1"
declare -x GTK_IM_MODULE="xim"
declare -x HOME="/home/ed"
declare -x IM_CONFIG_PHASE="1"
declare -x INSTANCE=""
declare -x JOB="dbus"
declare -x L
Re: [PATCH] Use __is_same_as for std::is_same and std::is_same_v
Le ven. 11 oct. 2019 à 17:50, Jonathan Wakely a écrit : > > By using the built-in we don't need to match a partial specialization > for std::is_same and don't need to instantiate std::is_same at all for > uses of std::is_same_v. > > * include/std/type_traits (is_same): Replace partial specialization > by using __is_same_as built-in in primary template. > (is_same_v): Use __is_same_as built-in instead of instantiating the > is_same trait. > > Tested x86_64-linux, committed to trunk. > Hi Jonathan, It looks like this creates the following error when I try to bootstrap clang 9.0.0 using the latest gcc and libstdc++ from trunk. Note that with g++, there is no problem, however it looks like clang++ has some problem with the new header. I don't know if this is an issue on libstdc++ side or clang++ side. Cheers, Romain FAILED: compiler-rt/lib/xray/CMakeFiles/RTXray.x86_64.dir/xray_buffer_queue.cc.o /workdir/build/final-system/llvm-build/./bin/clang++ --target=x86_64-1a-linux-gnu -DXRAY_HAS_EXCEPTIONS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/workdir/src/llvm-9.0.0/compiler-rt/lib/xray/.. -I/workdir/src/llvm-9.0.0/compiler-rt/lib/xray/../../include -O2 -msse -msse2 -msse3 -I/workdir/build/final-system/llvm-temporary-static-dependencies/install/include -I/workdir/build/final-system/llvm-temporary-static-dependencies/install/include/ncursesw -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -std=c++11 -Wno-unused-parameter -O3 -DNDEBUG-m64 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fno-rtti -MD -MT compiler-rt/lib/xray/CMakeFiles/RTXray.x86_64.dir/xray_buffer_queue.cc.o -MF compiler-rt/lib/xray/CMakeFiles/RTXray.x86_64.dir/xray_buffer_queue.cc.o.d -o compiler-rt/lib/xray/CMakeFiles/RTXray.x86_64.dir/xray_buffer_queue.cc.o -c /workdir/src/llvm-9.0.0/compiler-rt/lib/xray/xray_buffer_queue.cc In file included from /workdir/src/llvm-9.0.0/compiler-rt/lib/xray/xray_buffer_queue.cc:21: In file included from /workdir/src/llvm-9.0.0/compiler-rt/lib/xray/xray_allocator.h:29: In file included from /workdir/src/llvm-9.0.0/compiler-rt/lib/xray/xray_utils.h:20: In file included from /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/utility:70: In file included from /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_pair.h:59: In file included from /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/move.h:55: /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/type_traits:1393:51: error: '_Tp' does not refer to a value : public integral_constant ^ /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/type_traits:1391:21: note: declared here template ^ /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/type_traits:1393:56: error: '_Up' does not refer to a value : public integral_constant ^ /opt/1A/toolchain/x86_64-v20.0.3/lib64/gcc/x86_64-1a-linux-gnu/10.0.0/../../../../include/c++/10.0.0/type_traits:1391:35: note: declared here template ^ 2 errors generated.
Re: [PATCH] Use __is_same_as for std::is_same and std::is_same_v
Le sam. 12 oct. 2019 à 17:44, Romain Geissler a écrit : > > It looks like this creates the following error when I try to bootstrap > clang 9.0.0 using the latest gcc and libstdc++ from trunk. Note that > with g++, there is no problem, however it looks like clang++ has some > problem with the new header. I don't know if this is an issue on > libstdc++ side or clang++ side. __is_same_as is not implemented as a compiler builtin in clang++ unlike g++, thus the error on clang 9.0.0 side. It looks like current clang trunk doesn't define __is_same_as either. Until clang implements this support (if it ever will), usage of __is_same_as in libstdc++ shall be conditional, only when __has_builtin(__is_same_as) is true (however that would require that gcc implements __has_builtin, as implemented here: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg00062.html ) Cheers, Romain
Re: [PATCH 2/2] Documentation hyperlinks for [-Wname-of-option] (PR 87488)
On Sat, Oct 12, 2019 at 10:21:56AM -0400, David Malcolm wrote: > It seems like instead it's displaying this: > > https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wshadow=local-Wshadow=local%1B]8 That's what it does for me (well, with weird escapes at both ends). And if run inside a tmux it just shows what you wanted to show, but there is no way to click it: the URL is not displayed at all. > What happens if you try this at the terminal: > > printf '\e]8;;http://example.com\e\\This is a link\e]8;;\e\\\n' In gnome term it shows a bunch of funny escapes. In xterm it just displays "This is a link", without any way to click it, like in the tmux scenario. The links are wrong btw, they should be version-specific? > I don't yet know if there's a way to query the terminal to see if the > escape is supported. My hope was that they would be silently > discarded. Just display the URL if you want this? That works, and has always worked, and many terminals have convenient ways to use that (have had that since forever, too). Can we have this *off* by default, please? The warnings are getting way too verbose. Often one warning does not fit on one screen already! > > Do you see a way to find out if the escape sequences are supported, > > or how to disable this function per configure option? > > Do you mean a GCC configure-time option to change the default? We have > that for colors; Do we? I've had GCC_COLORS= since forever; is this finally not needed anymore? Neat :-) Now just ASAN_OPTIONS=color=never TSAN_OPTIONS=color=never by default and there is a bit of sanity again. > Is this working/broken for other people? I've only tried your example so far, and that does not work :-( Segher
Re: [PATCH] Fix PR c++/92024
On 10/11/19 6:31 PM, Jason Merrill wrote: > On 10/10/19 2:06 PM, Bernd Edlinger wrote: >> On 10/10/19 7:49 PM, Jason Merrill wrote: >>> On 10/10/19 10:42 AM, Bernd Edlinger wrote: Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C >>> >>> Why does that flag cause this crash? >>> >> >> gcc/cp/name-lookup.c: >> >> if (warn_shadow) >> warning_code = OPT_Wshadow; >> else if (warn_shadow_local) >> warning_code = OPT_Wshadow_local; >> else if (warn_shadow_compatible_local >> && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) >> || (!dependent_type_p (TREE_TYPE (decl)) >> && !dependent_type_p (TREE_TYPE (old)) >> /* If the new decl uses auto, we don't yet know >> its type (the old type cannot be using auto >> at this point, without also being >> dependent). This is an indication we're >> (now) doing the shadow checking too >> early. */ >> && !type_uses_auto (TREE_TYPE (decl)) >> && can_convert (TREE_TYPE (old), TREE_TYPE (decl), >> tf_none >> warning_code = OPT_Wshadow_compatible_local; >> >> if -Wshadow=compatible-local is used, the can_convert function crashes >> in instantiate_class_template_1. > > Right, checking can_convert is problematic here, as it can cause template > instantiations that change the semantics of the program. Or, in this case, > crash. > So I try to make C++ behave more consistently with the code in c-decl.c, thus dependent on warn_shadow but not on warn_shadow_local and/or warn_shadow_compatible_local: if (warn_shadow) warning_code = OPT_Wshadow; else if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) warning_code = OPT_Wshadow_compatible_local; else warning_code = OPT_Wshadow_local; warned = warning_at (DECL_SOURCE_LOCATION (new_decl), warning_code, "declaration of %qD shadows a parameter", new_decl); I cannot remove the if (warn_shadow) since this breaks gcc.dg/pr48062.c which uses: #pragma GCC diagnostic ignored "-Wshadow" to disable a -Wshadow=compatible-local warning, but while -Wno-shadow on the command line disables also dependent warnings the pragma does not (always) do that. So instead I'd like to adjust the doc of -Wshadow to reflect the implementation and remove the if(warn_shadow_local) to have C and C++ behave identical and hopefully now in sync with the doc. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. 2019-10-12 Bernd Edlinger * doc/invoke.texi (-Wshadow, -Wshadow=global -Wshadow=local, -Wshadow=compatible-local): Update documentation. cp: 2019-10-12 Bernd Edlinger PR c++/92024 * name-lookup.c (check_local_shadow): Shadowing TYPE_DECLs is always a -Wshadow=compatible-local warning, unless -Wshadow is used. testsuite: 2019-10-12 Bernd Edlinger PR c++/92024 * g++.dg/parse/crash70.C: New test. * c-c++-common/Wshadow-1.c: New test. Index: gcc/cp/name-lookup.c === --- gcc/cp/name-lookup.c (revision 276893) +++ gcc/cp/name-lookup.c (working copy) @@ -2750,29 +2750,31 @@ check_local_shadow (tree decl) parameter, more than often they would use the variable thinking (mistakenly) it's still the parameter. It would be rare that users would use the variable in the place that - expects the parameter but thinking it's a new decl. */ + expects the parameter but thinking it's a new decl. + If either object is a TYPE_DECL, '-Wshadow=compatible-local' + warns regardless of whether one of the types involved + is a subclass of the other, since that is never okay. */ enum opt_code warning_code; if (warn_shadow) warning_code = OPT_Wshadow; - else if (warn_shadow_local) - warning_code = OPT_Wshadow_local; - else if (warn_shadow_compatible_local - && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) - || (!dependent_type_p (TREE_TYPE (decl)) - && !dependent_type_p (TREE_TYPE (old)) - /* If the new decl uses auto, we don't yet know - its type (the old type cannot be using auto - at this point, without also being - dependent). This is an indication we're - (now) doing the shadow checking too - early. */ - && !type_uses_auto (TREE_TYPE (decl)) - && can_convert (TREE_TYPE (old), TREE_TYPE (decl), - tf_none + else if (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) + || TREE_CODE (decl) == TYPE_DECL + || TREE_CODE (old) == TYPE_DECL +
Re: [PATCH] Fix PR c++/92024
On 10/12/19 12:10 PM, Bernd Edlinger wrote: [snip snip] So instead I'd like to adjust the doc of -Wshadow to reflect the implementation and remove the if(warn_shadow_local) to have C and C++ behave identical and hopefully now in sync with the doc. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? The documentation changes look good to me. -Sandra
[committed] Fix warning compiling pa.c
This patch fixes a warning from pa.c.
Tested on hppa-unknown-linux-gnu. Committed to trunk.
Dave
Index: gcc/config/pa/pa.c
===
--- gcc/config/pa/pa.c (revision 276877)
+++ gcc/config/pa/pa.c (working copy)
@@ -535,7 +535,7 @@
if (! TARGET_GAS && write_symbols != NO_DEBUG)
{
- warning (0, "%<-g%> is only supported when using GAS on this
processor,");
+ warning (0, "%<-g%> is only supported when using GAS on this processor");
warning (0, "%<-g%> option disabled");
write_symbols = NO_DEBUG;
}
Re: [PATCH] The inline keyword is supported in all new C standards
On Wed, 25 Sep 2019 16:38:25 PDT (-0700), jos...@codesourcery.com wrote: On Tue, 24 Sep 2019, Palmer Dabbelt wrote: The documentation used to indicate that the inline keyword was only supported by c99 and c11, whereas in fact it is supported by c99 and all newer standards. gcc/ChangeLog 2019-09-24 Palmer Dabbelt * doc/extended.texi (Alternate Keywords): Change "-std=c11" to "a later standard." OK with the filename corrected in the ChangeLog entry. Committed (though I screwed up the changelog, sorry!)
Re: [patch, fortran] Fix PR 92004, restore Lapack compilation
Hi,
I think I have resolved all the issues (see attached patch and test
case).
Basically, the patch now walks through the refs and looks at the
latest thing that could be an array or a scalar.
Regarding CLASS in argument lists without an explicit interface:
I think that this is disallowed because an explicit interface
is required for a polymorphic dummy argument, and I see no
way of passing a polymorphic argument to a procedure without
having a polymorphic argument as a dummy argument.
While I was at it, I also changed some language to match the
language of the standard more closely.
As you can see in the test case, I tried to cover all relevant
cases.
Regression-tested. OK for trunk?
Regards
Thomas
2019-10-12 Thomas Koenig
PR fortran/92004
* array.c (expand_constructor): Set from_constructor on
expression.
* gfortran.h (gfc_symbol): Add maybe_array.
(gfc_expr): Add from_constructor.
* interface.c (maybe_dummy_array_arg): New function.
(compare_parameter): If the formal argument is generated from a
call, check the conditions where an array element could be
passed to an array. Adjust error message for assumed-shape
or pointer array. Use correct language for assumed shaped arrays.
(gfc_get_formal_from_actual_arglist): Set maybe_array on the
symbol if the actual argument is an array element fulfilling
the conditions of 15.5.2.4.
2019-10-12 Thomas Koenig
PR fortran/92004
* gfortran.dg/argument_checking_24.f90: New test.
* gfortran.dg/abstract_type_6.f90: Add error message.
* gfortran.dg/argument_checking_11.f90: Correct wording
in error message.
* gfortran.dg/argumeent_checking_13.f90: Likewise.
* gfortran.dg/interface_40.f90: Add error message.
Index: fortran/array.c
===
--- fortran/array.c (Revision 276506)
+++ fortran/array.c (Arbeitskopie)
@@ -1763,6 +1763,7 @@ expand_constructor (gfc_constructor_base base)
gfc_free_expr (e);
return false;
}
+ e->from_constructor = 1;
current_expand.offset = &c->offset;
current_expand.repeat = &c->repeat;
current_expand.component = c->n.component;
Index: fortran/gfortran.h
===
--- fortran/gfortran.h (Revision 276506)
+++ fortran/gfortran.h (Arbeitskopie)
@@ -1614,6 +1614,9 @@ typedef struct gfc_symbol
/* Set if a previous error or warning has occurred and no other
should be reported. */
unsigned error:1;
+ /* Set if the dummy argument of a procedure could be an array despite
+ being called with a scalar actual argument. */
+ unsigned maybe_array:1;
int refs;
struct gfc_namespace *ns; /* namespace containing this symbol */
@@ -2194,6 +2197,11 @@ typedef struct gfc_expr
/* Set this if no warning should be given somewhere in a lower level. */
unsigned int do_not_warn : 1;
+
+ /* Set this if the expression came from expanding an array constructor. */
+
+ unsigned int from_constructor : 1;
+
/* If an expression comes from a Hollerith constant or compile-time
evaluation of a transfer statement, it may have a prescribed target-
memory representation, and these cannot always be backformed from
Index: fortran/interface.c
===
--- fortran/interface.c (Revision 276506)
+++ fortran/interface.c (Arbeitskopie)
@@ -2229,6 +2229,64 @@ argument_rank_mismatch (const char *name, locus *w
}
+/* Under certain conditions, a scalar actual argument can be passed
+ to an array dummy argument - see F2018, 15.5.2.4, paragraph 14.
+ This function returns true for these conditions so that an error
+ or warning for this can be suppressed later. Always return false
+ for expressions with rank > 0. */
+
+bool
+maybe_dummy_array_arg (gfc_expr *e)
+{
+ gfc_symbol *s;
+ gfc_ref *ref;
+ bool array_pointer, assumed_shape, scalar_ref;
+
+ if (e->rank > 0)
+return false;
+
+ if (e->ts.type == BT_CHARACTER && e->ts.kind == 1)
+return true;
+
+ /* If this comes from a constructor, it has been an array element
+ originally. */
+
+ if (e->expr_type == EXPR_CONSTANT)
+return e->from_constructor;
+
+ if (e->expr_type != EXPR_VARIABLE)
+return false;
+
+ s = e->symtree->n.sym;
+
+ if (s->attr.dimension)
+array_pointer = s->attr.pointer;
+ else
+scalar_ref = true;
+
+ if (s->as && s->as->type == AS_ASSUMED_SHAPE)
+assumed_shape = true;
+
+ for (ref=e->ref; ref; ref=ref->next)
+{
+ if (ref->type == REF_COMPONENT)
+ {
+ symbol_attribute *attr;
+ attr = &ref->u.c.component->attr;
+ if (attr->dimension)
+ {
+ array_pointer = attr->pointer;
+ assumed_shape = false;
+ scalar_ref = false;
+ }
+ else
+ scalar_ref = true;
+ }
+}
+
+ return !(scalar_ref ||
[Darwin, committed] Suppress emitting empty ctor/dtor sections.
Older versions of GCC emit empty .constructor/.destructor sections
whenever building for C++. In fact, these sections are only used for
kernel mode code - so don't emit them unless that's what we're
building.
tested on x86_64-darwin16, powerpc-darwin9,
applied to mainline
thanks
Iain
gcc/ChangeLog:
2019-10-12 Iain Sandoe
* config/darwin.c (darwin_file_end): Only emit empty CTOR/DTOR
sections when building kernel extension code.
diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index 539ef759d3..ddce9f8c8b 100644
--- a/gcc/config/darwin.c
+++ b/gcc/config/darwin.c
@@ -2931,8 +2931,9 @@ darwin_file_end (void)
}
machopic_finish (asm_out_file);
- if (lang_GNU_CXX ())
+ if (flag_apple_kext)
{
+ /* These sections are only used for kernel code. */
switch_to_section (darwin_sections[constructor_section]);
switch_to_section (darwin_sections[destructor_section]);
ASM_OUTPUT_ALIGN (asm_out_file, 1);
[Darwin, machopic 5/n, committed] Make machopic_finish() static.
It's only called from darwin.c.
tested on x86_64-darwin16,
applied to mainline
thanks
Iain
gcc/ChangeLog:
2019-10-12 Iain Sandoe
* config/darwin-protos.h (machopic_finish): Delete.
* config/darwin.c (machopic_finish): Make static.
diff --git a/gcc/config/darwin-protos.h b/gcc/config/darwin-protos.h
index e5614b627d..afeca81f80 100644
--- a/gcc/config/darwin-protos.h
+++ b/gcc/config/darwin-protos.h
@@ -53,8 +53,6 @@ extern void darwin_set_default_type_attributes (tree);
#endif /* TREE_CODE */
-extern void machopic_finish (FILE *);
-
extern int machopic_reloc_rw_mask (void);
extern section *machopic_select_section (tree, int, unsigned HOST_WIDE_INT);
diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index ddce9f8c8b..8efb14ebdb 100644
--- a/gcc/config/darwin.c
+++ b/gcc/config/darwin.c
@@ -1188,7 +1188,7 @@ machopic_output_indirection (machopic_indirection **slot,
FILE *asm_out_file)
return 1;
}
-void
+static void
machopic_finish (FILE *asm_out_file)
{
if (machopic_indirections)
[Darwin, machopic 6/n, committed] Fix for 67183
When we're using the LLVM-based assembler (the default on modern Darwin)
the ordering of stubs and non-lazy symbol pointers is important.
Interleaving the output (current GCC behaviour) leads to crashes which
prevents us from building code with symbol stubs.
To resolve this, we order the output of stubs and symbol indirections:
1. Any indirections in the data section
2. Symbol stubs.
3. Non-lazy symbol pointers.
At present, we still emit LTO sections after these.
tested on x86_64-darwin16, i686-darwin9,
applied to mainline,
thanks
Iain
gcc/ChangeLog:
2019-10-12 Iain Sandoe
PR target/67183
* config/darwin.c (machopic_indirection): New field to flag
non-lazy-symbol-pointers in the data section.
(machopic_indirection_name): Compute if an indirection should
appear in the data section.
(machopic_output_data_section_indirection): New callback split
from machopic_output_indirection.
(machopic_output_stub_indirection): Likewise.
(machopic_output_indirection): Retain the code for non-lazy
symbol pointers in their regular section.
(machopic_finish): Use the new callbacks to order the indirection
output.
diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index 8efb14ebdb..f6543fc997 100644
--- a/gcc/config/darwin.c
+++ b/gcc/config/darwin.c
@@ -454,6 +454,13 @@ typedef struct GTY ((for_user)) machopic_indirection
bool stub_p;
/* True iff this stub or pointer has been referenced. */
bool used;
+ /* True iff a non-lazy symbol pointer should be emitted into the .data
+ section, rather than the non-lazy symbol pointers section. The cases
+ for which this occurred seem to have been unintentional, and later
+ toolchains emit all of the indirections to the 'usual' section. We
+ are keeping this in case it is necessary to preserve compatibility with
+ older toolchains. */
+ bool nlsp_in_data_section;
} machopic_indirection;
struct indirection_hasher : ggc_ptr_hash
@@ -590,6 +597,18 @@ machopic_indirection_name (rtx sym_ref, bool stub_p)
p->ptr_name = xstrdup (buffer);
p->stub_p = stub_p;
p->used = false;
+ /* Here we are undoing a number of causes that placed some indirections
+(apparently erroneously) into the .data section. Specifically, some
+symbols that are ABI mandated indirections and some hidden symbols
+were being placed there - which cause difficulties with later
+versions of ld64.
+ */
+ p->nlsp_in_data_section =
+ ! MACHO_SYMBOL_MUST_INDIRECT_P (sym_ref)
+ && ! MACHO_SYMBOL_HIDDEN_VIS_P (sym_ref)
+ && (machopic_symbol_defined_p (sym_ref)
+ || SYMBOL_REF_LOCAL_P (sym_ref))
+ && ! indirect_data (sym_ref);
*slot = p;
}
@@ -1069,121 +1088,140 @@ machopic_legitimize_pic_address (rtx orig,
machine_mode mode, rtx reg)
return pic_ref;
}
-/* Output the stub or non-lazy pointer in *SLOT, if it has been used.
- DATA is the FILE* for assembly output. Called from
- htab_traverse. */
+/* Callbacks to output the stub or non-lazy pointers.
+ Each works on the item in *SLOT,if it has been used.
+ DATA is the FILE* for assembly output.
+ Called from htab_traverses, invoked from machopic_finish(). */
int
-machopic_output_indirection (machopic_indirection **slot, FILE *asm_out_file)
+machopic_output_data_section_indirection (machopic_indirection **slot,
+ FILE *asm_out_file)
{
machopic_indirection *p = *slot;
- rtx symbol;
- const char *sym_name;
- const char *ptr_name;
- if (!p->used)
+ if (!p->used || !p->nlsp_in_data_section)
return 1;
- symbol = p->symbol;
- sym_name = XSTR (symbol, 0);
- ptr_name = p->ptr_name;
+ rtx symbol = p->symbol;
+ /* The original symbol name. */
+ const char *sym_name = XSTR (symbol, 0);
+ /* The name of the indirection symbol. */
+ const char *ptr_name = p->ptr_name;
- if (p->stub_p)
-{
- char *sym;
- char *stub;
- tree id;
+ switch_to_section (data_section);
+ assemble_align (GET_MODE_ALIGNMENT (Pmode));
+ assemble_label (asm_out_file, ptr_name);
+ assemble_integer (gen_rtx_SYMBOL_REF (Pmode, sym_name),
+ GET_MODE_SIZE (Pmode),
+ GET_MODE_ALIGNMENT (Pmode), 1);
- id = maybe_get_identifier (sym_name);
- if (id)
- {
- tree id_orig = id;
+ return 1;
+}
- while (IDENTIFIER_TRANSPARENT_ALIAS (id))
- id = TREE_CHAIN (id);
- if (id != id_orig)
- sym_name = IDENTIFIER_POINTER (id);
- }
+int
+machopic_output_stub_indirection (machopic_indirection **slot,
+ FILE *asm_out_file)
+{
+ machopic_indirection *p = *slot;
- sym = XALLOCAVEC (char, strlen (sym_name) + 2);
- if (sym_name[0] == '*' || sym_name[0] == '&')
- strcpy (sym, sym_name + 1);
- else if (sym_name[0] == '-
[committed]
This is the first patch in a series of changes intended to fix glibc/23296. This bug is a data race in the setting of function descriptors during lazy binding. If a descriptor is updated between the loading of the function target address and the PIC global pointer in another thread, _dl_runtime_resolve() is entered with the new global pointer instead of the expected reloc offset. _dl_runtime_resolve() could handle this situation if we modify the indirect call sequence to preserve the function pointer (descriptor address) in register %r22. We also change the code to consistently load the function address before the global pointer. There might be still an issue on PA 2.0 hardware due to the out-of-order execution of accesses. Currently, the linker doesn't double word align descriptors. As a result, descriptors can span cache lines and page boundaries. Thus, it might be possible that a function is entered with an incorrect global pointer. This patch slightly improves $$dyncall on Linux. Tested on hppa-unknown-linux-gnu. Committed to trunk and gcc-9 branch. 2019-10-12 John David Anglin * config/pa/lib2funcs.S (__gcc_plt_call): Load branch target to %r21. Load PIC register after branch target. Fix white space. * config/pa/milli64.S ($$dyncall): Separate LINUX and non LINUX implementations. Load PIC register after branch target. Don't clobber function pointer when it points to function descriptor. Use nullification instead of branch in LINUX implementation. Index: config/pa/lib2funcs.S === --- config/pa/lib2funcs.S (revision 276920) +++ config/pa/lib2funcs.S (working copy) @@ -55,13 +55,13 @@ ; An inline version of dyncall so we don't have to worry ; about long calls to millicode, PIC and other complexities. bb,>=,n %r22,30,L$foo -depi 0,31,2,%r22 -ldw 4(%r22),%r19 -ldw 0(%r22),%r22 + depi 0,31,2,%r22 + ldw 0(%r22),%r21 + ldw 4(%r22),%r19 L$foo -ldsid (%r22),%r1 -mtsp %r1,%sr0 -ble 0(%sr0,%r22) + ldsid (%r21),%r1 + mtsp %r1,%sr0 + ble 0(%sr0,%r21) copy %r31,%r2 ldw -8(%r30),%r2 Index: config/pa/milli64.S === --- config/pa/milli64.S (revision 276920) +++ config/pa/milli64.S (working copy) @@ -222,19 +222,26 @@ .proc .callinfo millicode .entry +#ifdef LINUX + extru,<>%r22,30,1,%r0 ; nullify if plabel bit set + bv,n%r0(%r22) ; branch to target + ldw -2(%r22),%r21 ; load address of target + bv %r0(%r21) ; branch to the real target + ldw 2(%r22),%r19; load new LTP value +#else bb,>=,n %r22,30,LREF(1) ; branch if not plabel address - depi0,31,2,%r22 ; clear the two least significant bits - ldw 4(%r22),%r19; load new LTP value - ldw 0(%r22),%r22; load address of target + ldw -2(%r22),%r21 ; load address of target to r21 + ldsid (%sr0,%r21),%r1 ; get the "space ident" selected by r21 + ldw 2(%r22),%r19; load new LTP value + mtsp%r1,%sr0; move that space identifier into sr0 + be 0(%sr0,%r21); branch to the real target + stw %r2,-24(%r30) ; save return address into frame marker LSYM(1) -#ifdef LINUX - bv %r0(%r22) ; branch to the real target -#else ldsid (%sr0,%r22),%r1 ; get the "space ident" selected by r22 mtsp%r1,%sr0; move that space identifier into sr0 - be 0(%sr0,%r22); branch to the real target + be 0(%sr0,%r22); branch to the target + stw %r2,-24(%r30) ; save return address into frame marker #endif - stw %r2,-24(%r30) ; save return address into frame marker .exit .procend #endif
[committed] Update indirect call sequences to preserve descriptor address in register %r22
This is the second patch in a series of changes intended to fix glibc/23296.
This bug
is a data race in the setting of function descriptors during lazy binding. If
a descriptor
is updated between the loading of the function target address and the PIC
global pointer
in another thread, _dl_runtime_resolve() is entered with the new global pointer
instead
of the expected reloc offset. _dl_runtime_resolve() could handle this situation
if we modify
the indirect call sequence to preserve the function pointer (descriptor
address) in register
%r22.
We also change the code to consistently load the function address before the
global pointer.
There might be still an issue on PA 2.0 hardware due to the out-of-order
execution of accesses.
Currently, the linker doesn't double word align descriptors. As a result,
descriptors can span
cache lines and page boundaries. Thus, it might be possible that a function is
entered with an
incorrect global pointer.
This patch updates the inline indirect call sequences generated in pa.c to
preserve the function
pointer in register %r22. The 32-bit trampoline sequence is updated because
%r22 no longer
contains the trampoline address on entry.
Tested on hppa-unknown-linux-gnu. Committed to trunk and gcc-9 branch.
Dave
2019-10-12 John David Anglin
* config/pa/pa.c (pa_output_call): Load descriptor address to register
%r22. Load function address before global pointer.
(pa_attr_length_indirect_call): Adjust length of inline versions of
$$dyncall.
(pa_output_indirect_call): Remove fast inline version of $$dyncall
before normal cases. Update inline $$dyncall sequences to preserve
function descriptor address in register %r22.
(TRAMPOLINE_CODE_SIZE): Adjust.
(pa_asm_trampoline_template): Revise 32-bit trampoline. Don't assume
register %r22 contains trampoline address.
(pa_trampoline_init): Adjust offsets.
(pa_trampoline_adjust_address): Likewise.
* config/pa/pa.h (TRAMPOLINE_SIZE): Adjust 32-bit size.
Index: config/pa/pa.c
===
--- config/pa/pa.c (revision 276920)
+++ config/pa/pa.c (working copy)
@@ -8027,20 +8027,22 @@
{
output_asm_insn ("addil LT'%0,%%r19", xoperands);
output_asm_insn ("ldw RT'%0(%%r1),%%r1", xoperands);
- output_asm_insn ("ldw 0(%%r1),%%r1", xoperands);
+ output_asm_insn ("ldw 0(%%r1),%%r22", xoperands);
}
else
{
output_asm_insn ("addil LR'%0-$global$,%%r27",
xoperands);
- output_asm_insn ("ldw RR'%0-$global$(%%r1),%%r1",
+ output_asm_insn ("ldw RR'%0-$global$(%%r1),%%r22",
xoperands);
}
- output_asm_insn ("bb,>=,n %%r1,30,.+16", xoperands);
- output_asm_insn ("depi 0,31,2,%%r1", xoperands);
- output_asm_insn ("ldw 4(%%sr0,%%r1),%%r19", xoperands);
- output_asm_insn ("ldw 0(%%sr0,%%r1),%%r1", xoperands);
+ output_asm_insn ("bb,>=,n %%r22,30,.+16", xoperands);
+ output_asm_insn ("depi 0,31,2,%%r22", xoperands);
+ /* Should this be an ordered load to ensure the target
+address is loaded before the global pointer? */
+ output_asm_insn ("ldw 0(%%r22),%%r1", xoperands);
+ output_asm_insn ("ldw 4(%%r22),%%r19", xoperands);
if (!sibcall && !TARGET_PA_20)
{
@@ -8133,10 +8135,6 @@
if (TARGET_PORTABLE_RUNTIME)
return 16;
- /* Inline version of $$dyncall. */
- if ((TARGET_NO_SPACE_REGS || TARGET_PA_20) && !optimize_size)
-return 20;
-
if (!TARGET_LONG_CALLS
&& ((TARGET_PA_20 && !TARGET_SOM && distance < 760)
|| distance < MAX_PCREL17F_OFFSET))
@@ -8146,13 +8144,16 @@
if (!flag_pic)
return 12;
- /* Inline version of $$dyncall. */
- if (TARGET_NO_SPACE_REGS || TARGET_PA_20)
-return 20;
-
+ /* Inline versions of $$dyncall. */
if (!optimize_size)
-return 36;
+{
+ if (TARGET_NO_SPACE_REGS)
+ return 28;
+ if (TARGET_PA_20)
+ return 32;
+}
+
/* Long PIC pc-relative call. */
return 20;
}
@@ -8189,22 +8190,6 @@
return "blr %%r0,%%r2\n\tbv,n %%r0(%%r31)";
}
- /* Maybe emit a fast inline version of $$dyncall. */
- if ((TARGET_NO_SPACE_REGS || TARGET_PA_20) && !optimize_size)
-{
- output_asm_insn ("bb,>=,n %%r22,30,.+12\n\t"
- "ldw 2(%%r22),%%r19\n\t"
- "ldw -2(%%r22),%%r22", xoperands);
- pa_output_arg_descriptor (insn);
- if (TARGET_NO_SPACE_REGS)
- {
- if (TARGET_PA_20)
-
[RFC, Darwin, PPC] Fix PR 65342.
Hi Folks,
(this is a bug reported against Fortran, but actually is a generic insn
selection problem for m64 powerpc-darwin. In fact, I’d say it renders
the 64b multilib unusable).
The solution proposed is Darwin-local (it's a long-standing bug and it
would be intended to back-port it). However, I'd welcome views on it.
The current Darwin load/store lo_sum patterns have neither predicate nor
constraint. This means that most parts of the backend, which rely on
recog() to validate the rtx, can produce invalid combinations/selections.
For 32bit cases this isn't a problem since we can load/store to unaligned
addresses using D-mode insns.
Conversely, for 64bit instructions that use DS mode, this can manifest as
assemble errors (for an assembler that checks the LO14 relocations), or as
crashes caused by wrong offsets (or worse, wrong content for the two LSBs).
Trying to find the right place to fix this has been tricky, there's
discussion in the PR trail. There doesn't seem to be any way to deal with
this in legitimize_address (since most of the damage is done by the wide open
patterns that are in effect after the legitimizer has run). Likewise,
extending the checks in legitimate_address_p() and mode_dependent_address
doesn't help if the constraint is not accurate in the end.
What we want to check for "Y" on Darwin is:
- that the alignment of the Symbols' target is sufficient for DS mode
- that the offset is suitable for DS mode.
(while looking through the Mach-O PIC unspecs).
Proposed:
1) Drop the Darwin-specific lo_sum patterns. Actually, this produces an
immediate progression in quite a number of tests, we begin using the
movdi_internal64 patterns. However there are also regressions caused by
instructions unable to satisfy their constraints.
2) To resolve this we need to extend the handling of the mem_operand_gpr to
allow looking through Mach-O PIC UNSPECs in the lo_sum cases.
- note, that rs6000_offsettable_memref_p () will not handle these so that
would return early, producing the issue with unsatisfiable constraints.
- I do wonder if that's also the case for some non-Darwin lo_sum cases.
(some things might be hard to detect, since the code will generally fall
back to doing " la Rx xxx@l ; ld Ry 0(Rx)" so it won't fail - just be
less efficient than it could be).
I’ve tested this on powerpc-darwin9, and powerpc64-linux-gnu.
It progresses around 120 tests across the testsuite (many in Fortran
but also in C and C++)
thoughts?
Iain
P.S. Probably the other crufty old lo_sum patterns can go too - but for another
day.
gcc/ChangeLog:
2019-10-12 Iain Sandoe
* config/rs6000/darwin.md (movdi_low, movsi_low_st): Delete
(movdi_low_st): Delete.
* config/rs6000/rs6000.c
(darwin_rs6000_legitimate_lo_sum_const_p): New.
(mem_operand_gpr): Validate Mach-O LO_SUM cases separately.
* config/rs6000/rs6000.md (movsi_low): Delete.
diff --git a/gcc/config/rs6000/darwin.md b/gcc/config/rs6000/darwin.md
index 3994447f38..16f710b28b 100644
--- a/gcc/config/rs6000/darwin.md
+++ b/gcc/config/rs6000/darwin.md
@@ -122,33 +122,6 @@ You should have received a copy of the GNU General Public
License
[(set_attr "type" "store")])
;; 64-bit MachO load/store support
-(define_insn "movdi_low"
- [(set (match_operand:DI 0 "gpc_reg_operand" "=r,*!d")
-(mem:DI (lo_sum:DI (match_operand:DI 1 "gpc_reg_operand" "b,b")
- (match_operand 2 "" ""]
- "TARGET_MACHO && TARGET_64BIT"
- "@
- ld %0,lo16(%2)(%1)
- lfd %0,lo16(%2)(%1)"
- [(set_attr "type" "load")])
-
-(define_insn "movsi_low_st"
- [(set (mem:SI (lo_sum:SI (match_operand:SI 1 "gpc_reg_operand" "b")
- (match_operand 2 "" "")))
- (match_operand:SI 0 "gpc_reg_operand" "r"))]
- "TARGET_MACHO && ! TARGET_64BIT"
- "stw %0,lo16(%2)(%1)"
- [(set_attr "type" "store")])
-
-(define_insn "movdi_low_st"
- [(set (mem:DI (lo_sum:DI (match_operand:DI 1 "gpc_reg_operand" "b,b")
- (match_operand 2 "" "")))
- (match_operand:DI 0 "gpc_reg_operand" "r,*!d"))]
- "TARGET_MACHO && TARGET_64BIT"
- "@
- std %0,lo16(%2)(%1)
- stfd %0,lo16(%2)(%1)"
- [(set_attr "type" "store")])
;; Mach-O PIC.
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index d1434a9f74..bc22573174 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7329,6 +7329,103 @@ address_offset (rtx op)
return NULL_RTX;
}
+/* This tests that a lo_sum {constant, symbol, symbol+offset} is valid for
+ the mode. If we can't find (or don't know) the alignment of the symbol
+ we assume (optimistically) that it's sufficiently aligned [??? maybe we
+ should be pessimistic]. Offsets are validated in the same way as for
+ reg + offset. */
+static bool
+darwin_rs6000_legitimate_lo_sum_const_p (rtx x, machine_mode mode)
+{
+ /* We should not get here with this. */
+ gcc_checking_assert (! mode
Re: [CFI, Darwin] A hook to allow target-handling of Personality and LSDA indirections.
I hadn’t realised it had been so long since I posted this:
ping.
Iain Sandoe wrote:
> If an assembler is used that supports .cfi_xxx, then (usually) GCC's
> configuration will detect this and try to use it to generate frame insns.
>
> This will not work for Darwin since the default implementation for the
> personality and LSDA indirection is not correct for Mach-O ABI.
>
> As of now we are currently “lucky” in that the Darwin implementation of
> objdump (based on LLVM) does not recognise “-Wf” (and also has slightly
> different whitespace output) which means that even tho most of the modern
> assemblers for Darwin *do* consume .cfi_ instructions, the GCC configure
> is not detecting this and we are falling back to the old direct DWARF output
> for CFA/FDEs.
>
> There is no point in fixing the configure, until the codegen will produce
> useful output.
>
> So, this patch introduces a target hook to allow target-specific processing
> of personality and LSDA indirections (if the hook is not populated, the
> default implementation is used).
>
> Once this fixed is applied it is possible to make progress towards
> modernising the Darwin handling of the frame insns (and, hopefully, with the
> newer dsymutil implementations to progress towards consuming DWARF4, at
> least).
>
> OK for trunk?
> thanks
> Iain
>
> gcc/ChangeLog:
>
> 2019-09-16 Iain Sandoe
>
> * config/darwin-protos.h (darwin_make_eh_symbol_indirect): New.
> * config/darwin.c (darwin_make_eh_symbol_indirect): New.
> * config/darwin.h (TARGET_ASM_MAKE_EH_SYMBOL_INDIRECT): New hook.
> * doc/tm.texi: Regenerated.
> * doc/tm.texi.in: Declare new hook.
> * dwarf2out.c (dwarf2out_do_cfi_startproc): Allow for target hook to
> alter the code gen
> for personality and LSDA indirections.
> * target.def: Describe new hook.
>
>
> diff --git a/gcc/config/darwin-protos.h b/gcc/config/darwin-protos.h
> index e5614b627d..bb3fe19e0d 100644
> --- a/gcc/config/darwin-protos.h
> +++ b/gcc/config/darwin-protos.h
> @@ -68,6 +68,7 @@ extern void darwin_non_lazy_pcrel (FILE *, rtx);
>
> extern void darwin_emit_unwind_label (FILE *, tree, int, int);
> extern void darwin_emit_except_table_label (FILE *);
> +extern rtx darwin_make_eh_symbol_indirect (rtx, bool);
>
> extern void darwin_pragma_ignore (struct cpp_reader *);
> extern void darwin_pragma_options (struct cpp_reader *);
> diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
> index e62a5c6595..7ef73cc58b 100644
> --- a/gcc/config/darwin.c
> +++ b/gcc/config/darwin.c
> @@ -2157,6 +2157,18 @@ darwin_emit_except_table_label (FILE *file)
> except_table_label_num++);
> ASM_OUTPUT_LABEL (file, section_start_label);
> }
> +
> +rtx
> +darwin_make_eh_symbol_indirect (rtx orig, bool ARG_UNUSED (pubvis))
> +{
> + if (DARWIN_PPC == 0 && TARGET_64BIT)
> +return orig;
> +
> + return gen_rtx_SYMBOL_REF (Pmode,
> + machopic_indirection_name (orig,
> + /*stub_p=*/false));
> +}
> +
> /* Generate a PC-relative reference to a Mach-O non-lazy-symbol. */
>
> void
> diff --git a/gcc/config/darwin.h b/gcc/config/darwin.h
> index fde191d5c4..06e6f0c3da 100644
> --- a/gcc/config/darwin.h
> +++ b/gcc/config/darwin.h
> @@ -593,6 +593,9 @@ extern GTY(()) int darwin_ms_struct;
> /* Emit a label to separate the exception table. */
> #define TARGET_ASM_EMIT_EXCEPT_TABLE_LABEL darwin_emit_except_table_label
>
> +/* Make an EH (personality or LDSA) symbol indirect as needed. */
> +#define TARGET_ASM_MAKE_EH_SYMBOL_INDIRECT darwin_make_eh_symbol_indirect
> +
> /* Our profiling scheme doesn't LP labels and counter words. */
>
> #define NO_PROFILE_COUNTERS 1
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 0b5a08d490..635faa22dc 100644
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index a9200551ed..c6d457cb5e 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -6445,6 +6445,8 @@ the jump-table.
>
> @hook TARGET_ASM_UNWIND_EMIT
>
> +@hook TARGET_ASM_MAKE_EH_SYMBOL_INDIRECT
> +
> @hook TARGET_ASM_UNWIND_EMIT_BEFORE_INSN
>
> @node Exception Region Output
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index aa7fd7eb46..b6d25c109a 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -988,7 +988,12 @@ dwarf2out_do_cfi_startproc (bool second)
>in the assembler. Further, the assembler can't handle any
>of the weirder relocation types. */
> if (enc & DW_EH_PE_indirect)
> - ref = dw2_force_const_mem (ref, true);
> + {
> + if (targetm.asm_out.make_eh_symbol_indirect != NULL)
> + ref = targetm.asm_out.make_eh_symbol_indirect (ref, true);
> + else
> + ref = dw2_force_const_mem (ref, true);
> + }
>
> fprintf (asm_out_file, "\t.cfi_personality %#x,", enc);
> output_addr_const (asm_out_file, ref);
> @@ -1006,7 +1011,12 @@ dwarf2out_do_cfi_sta
Re: [RFC, Darwin, PPC] Fix PR 65342.
Hi! On Sat, Oct 12, 2019 at 10:13:16PM +0100, Iain Sandoe wrote: > (this is a bug reported against Fortran, but actually is a generic insn > selection problem for m64 powerpc-darwin. In fact, I’d say it renders > the 64b multilib unusable). > > The solution proposed is Darwin-local (it's a long-standing bug and it > would be intended to back-port it). However, I'd welcome views on it. > > The current Darwin load/store lo_sum patterns have neither predicate nor > constraint. This means that most parts of the backend, which rely on > recog() to validate the rtx, can produce invalid combinations/selections. > > For 32bit cases this isn't a problem since we can load/store to unaligned > addresses using D-mode insns. Can you? -m32 -mpowerpc64? We did have a bug with this before, maybe six years ago or so... Alan, do you remember? It required some assembler work IIRC. > Conversely, for 64bit instructions that use DS mode, this can manifest as > assemble errors (for an assembler that checks the LO14 relocations), or as > crashes caused by wrong offsets (or worse, wrong content for the two LSBs). Resulting in a different insn than intended. Yeah. > Trying to find the right place to fix this has been tricky, there's > discussion in the PR trail. There doesn't seem to be any way to deal with > this in legitimize_address (since most of the damage is done by the wide open > patterns that are in effect after the legitimizer has run). Likewise, > extending the checks in legitimate_address_p() and mode_dependent_address > doesn't help if the constraint is not accurate in the end. > > What we want to check for "Y" on Darwin is: > - that the alignment of the Symbols' target is sufficient for DS mode > - that the offset is suitable for DS mode. > (while looking through the Mach-O PIC unspecs). > > Proposed: > > 1) Drop the Darwin-specific lo_sum patterns. Actually, this produces an > immediate progression in quite a number of tests, we begin using the > movdi_internal64 patterns. However there are also regressions caused by > instructions unable to satisfy their constraints. And code quality regressions? Or does it even improve? (One can dream...) > 2) To resolve this we need to extend the handling of the mem_operand_gpr to > allow looking through Mach-O PIC UNSPECs in the lo_sum cases. > > - note, that rs6000_offsettable_memref_p () will not handle these so that >would return early, producing the issue with unsatisfiable constraints. > > - I do wonder if that's also the case for some non-Darwin lo_sum cases. Not sure what exactly you mean here? Non-Darwin doesn't have issues with not looking through Darwin-specific unspecs, anywhere, so you mean something more general no doubt -- but what? > (some things might be hard to detect, since the code will generally fall > back to doing " la Rx xxx@l ; ld Ry 0(Rx)" so it won't fail - just be > less efficient than it could be). Oh, you are saying mem_operand_gpr does not allow lo_sum *at all*? > + /* If we don't know the alignment of the thing to which the symbol refers, > + we assume optimistically it is "enough". > + ??? maybe we should be pessimistic instead. */ > + unsigned align = 0; If you guess it is aligned enough but it isn't, the compile will fail. Not good. OTOH, when don't we know the alignment? Only for globals? Does the ABI guarantee alignment in that case? I'll have another looke through this (esp. the generic part) when I'm fresh awake (but not before coffee!). Alan, can you have a look as well please? Segher
Re: [RFC, Darwin, PPC] Fix PR 65342.
Hi, Segher Boessenkool wrote: On Sat, Oct 12, 2019 at 10:13:16PM +0100, Iain Sandoe wrote: (this is a bug reported against Fortran, but actually is a generic insn selection problem for m64 powerpc-darwin. In fact, I’d say it renders the 64b multilib unusable). The solution proposed is Darwin-local (it's a long-standing bug and it would be intended to back-port it). However, I'd welcome views on it. The current Darwin load/store lo_sum patterns have neither predicate nor constraint. This means that most parts of the backend, which rely on recog() to validate the rtx, can produce invalid combinations/selections. For 32bit cases this isn't a problem since we can load/store to unaligned addresses using D-mode insns. Can you? -m32 -mpowerpc64? In principle, this ought to be supported (there’s a pattern for the specific case in darwin.md) however, I don’t think it’s tested and there seem to be a number of places where the conditional is TARGET_64BIT instead of TARGET_POWERPC64 so my guess is that there would be some work to do to make it happen, We did have a bug with this before, maybe six years ago or so... Alan, do you remember? It required some assembler work IIRC. Conversely, for 64bit instructions that use DS mode, this can manifest as assemble errors (for an assembler that checks the LO14 relocations), or as crashes caused by wrong offsets (or worse, wrong content for the two LSBs). Resulting in a different insn than intended. Yeah. Trying to find the right place to fix this has been tricky, there's discussion in the PR trail. There doesn't seem to be any way to deal with this in legitimize_address (since most of the damage is done by the wide open patterns that are in effect after the legitimizer has run). Likewise, extending the checks in legitimate_address_p() and mode_dependent_address doesn't help if the constraint is not accurate in the end. What we want to check for "Y" on Darwin is: - that the alignment of the Symbols' target is sufficient for DS mode - that the offset is suitable for DS mode. (while looking through the Mach-O PIC unspecs). Proposed: 1) Drop the Darwin-specific lo_sum patterns. Actually, this produces an immediate progression in quite a number of tests, we begin using the movdi_internal64 patterns. However there are also regressions caused by instructions unable to satisfy their constraints. And code quality regressions? Or does it even improve? (One can dream…) The code quality for the testcases I made for this is good when we do the part below - because, unless we cater for the indirections, we generate quite a few unnecessary loads for the PIC case. 2) To resolve this we need to extend the handling of the mem_operand_gpr to allow looking through Mach-O PIC UNSPECs in the lo_sum cases. - note, that rs6000_offsettable_memref_p () will not handle these so that would return early, producing the issue with unsatisfiable constraints. - I do wonder if that's also the case for some non-Darwin lo_sum cases. Not sure what exactly you mean here? Non-Darwin doesn't have issues with not looking through Darwin-specific unspecs, anywhere, so you mean something more general no doubt -- but what? LO_SUM appears to be handled in the case that it refers to a constant pool address, but I wonder if there are any other circumstances that it could be relevant. (some things might be hard to detect, since the code will generally fall back to doing " la Rx xxx@l ; ld Ry 0(Rx)" so it won't fail - just be less efficient than it could be). Oh, you are saying mem_operand_gpr does not allow lo_sum *at all*? No, I think it will let LO_SUM through for constant pool entries, I’m not sure about any other case tho (perhaps there is no other case that matters). The absence of something is much harder to reason about / test for than the presence of something - but I am sure that you and Alan know what you expect to be there. + /* If we don't know the alignment of the thing to which the symbol refers, + we assume optimistically it is "enough". + ??? maybe we should be pessimistic instead. */ + unsigned align = 0; If you guess it is aligned enough but it isn't, the compile will fail. Not good. OTOH, when don't we know the alignment? Only for globals? Does the ABI guarantee alignment in that case? We usually know the alignment for globals too - I’m not sure under what circumstance there is no decl attached to the symbol_ref There is code rs6000.c:7627 or so, that suggests that section anchors can produce the effect, so that’s something I can look at once the basic problem is solved. I'll have another looke through this (esp. the generic part) when I'm fresh awake (but not before coffee!). Alan, can you have a look as well please? thanks! Iain
Re: [PATCH][AArch64] Fix symbol offset limit
On 10/12/19 3:56 AM, Richard Sandiford wrote: > Wilco Dijkstra writes: >> Hi Richard, >> >>> If global_char really is a char then isn't that UB? >> >> No why? > > Well, "simple expressions like &global_char + 0xff00" made it sound > like there really was a: > >extern char global_char; > > Only &global_char and &global_char + 1 are defined in that case. > I was probably taking the example too literally though. > >> We can do all kinds of arithmetic based on pointers, either using >> pointer types or converted to uintptr_t. Note that the optimizer >> actually creates these expressions, for example arr[N-x] can be >> evaluated as (&arr[0] + N) - x. So this remains legal even if N is >> well outside the bounds of the array. > > Yeah, true, and I remember valid "x[n - big_offset]" -> invalid > "x + big_offset - n" being a problem for MIPS too. It was one of > the things offset_within_block_p was helping to avoid. Multiple targets struggle with this. PA, mn103 also come to mind immediately. [ ... ] > > But more importantly, we can't say definitively that code quality isn't > affected, only that it wasn't affected for the cases we've looked at. > People could patch the compiler if the new ranges turn out not to strike > the right balance for their use cases, but not everyone wants to do that. Absolutely. IIRC These kinds of addresses are regularly created for Ada. Or at least they were. One of the concepts that really helped the PA from a codegen standpoint was to distinguish between pointers which we know point into the object from those which might point outside the object. Given that knowledge we could generate more efficient code for the former (particularly WRT indexed and scaled indexed address modes) and punt the latter. REGNO_POINTER_FLAG carries this information. Jeff
