Re: [PATCH] libgccjit: Handle truncation and extension for casts [PR 95498]
Hi.
I'd like to know what's the status of the review for this patch.
(Same for my other patch 96889: add some reflection functions in the jit
C api)
Thanks.
On Tue, Jul 21, 2020 at 11:29:57PM +0200, Andrea Corallo wrote:
Hi Antoni,
a couple of nits and some thoughts.
Antoni Boucher via Gcc-patches writes:
2020-07-12 Antoni Boucher
gcc/jit/
PR target/95498
* jit-playback.c: Add support to handle truncation and extension
^^^
here we usually add the function that gets
modified, you can look at other changelog entries as example.
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 0fddf04da87..4f4a1080c36 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -61,22 +61,39 @@ along with GCC; see the file COPYING3. If not see
/* gcc::jit::playback::context::build_cast uses the convert.h API,
which in turn requires the frontend to provide a "convert"
- function, apparently as a fallback.
-
- Hence we provide this dummy one, with the requirement that any casts
- are handled before reaching this. */
+ function, apparently as a fallback for casts that can be simplified
+ (truncation, extension). */
extern tree convert (tree type, tree expr);
tree
convert (tree dst_type, tree expr)
{
- gcc_assert (gcc::jit::active_playback_ctxt);
- gcc::jit::active_playback_ctxt->add_error (NULL, "unhandled conversion");
- fprintf (stderr, "input expression:\n");
- debug_tree (expr);
- fprintf (stderr, "requested type:\n");
- debug_tree (dst_type);
- return error_mark_node;
+ tree t_ret = NULL;
+ t_ret = targetm.convert_to_type (dst_type, expr);
+ if (t_ret)
+ return t_ret;
^^^
indent nit
+ enum tree_code dst_code = TREE_CODE (dst_type);
+ switch (dst_code)
+{
+case INTEGER_TYPE:
+case ENUMERAL_TYPE:
+ t_ret = convert_to_integer (dst_type, expr);
+ goto maybe_fold;
+
+default:
+ gcc_assert (gcc::jit::active_playback_ctxt);
+ gcc::jit::active_playback_ctxt->add_error (NULL, "unhandled conversion");
+ fprintf (stderr, "input expression:\n");
+ debug_tree (expr);
+ fprintf (stderr, "requested type:\n");
+ debug_tree (dst_type);
+ return error_mark_node;
+
+maybe_fold:
+ if (TREE_CODE (t_ret) != C_MAYBE_CONST_EXPR)
+ t_ret = fold (t_ret);
+ return t_ret;
+}
}
Looking at 'convert' at c-convert.c:66 the INTEGER_TYPE case here looks
good, but given the set of casts we accept as input I guess we should
handle also POINTER_TYPE, BOOLEAN_TYPE and REAL_TYPE. What do you think
about?
Hope it helps
Bests
Andrea
Re: Patch for PR analyzer/98797
On Wed, 2021-02-10 at 19:42 +, brian.sobulefsky wrote:
Hi Brian
Thanks for the patch.
The patch is large enough to count as legally significant; I've sent
you information on copyright assignment separately; the patch can't be
committed until that paperwork is in place.
In the meantime, I've added some review notes inline below throughout:
> Address the bug found in test file
> gcc/testsuite/gcc.dg/analyzer/casts-1.c, as
> recorded by the XFAIL, and some simpler and more complex versions found
> during
> the investigation of it. PR analyzer/98797 was opened for these bugs.
>
> The simplest manifestation of this bug can be replicated with:
>
> char arr[] = {'A', 'B', 'C', 'D'};
> char *pt = ar;
> __analyzer_eval(*(pt + 0) == 'A');
> __analyzer_eval(*(pt + 1) == 'B');
> //etc
>
> The result of the eval call is "UNKNOWN" because the analyzer is unable to
> determine the value at the pointer. Note that array access (such as
> arr[0]) is
> correctly handled. The code responsible for this is in file
> region-model-manager.cc, function
> region_model_manager::maybe_fold_sub_svalue.
> The relevant section is commented /* Handle getting individual chars from
> STRING_CST */. This section only had a case for an element_region. A case
> needed to be added for an offset_region.
>
> Additionally, when the offset was 0, such as in *pt or *(pt + 0), the
> function
> region_model_manager::get_offset_region was failing to make the needed
> offset
> region at all. This was due to the test for a constant 0 pointer that was
> then
> returning get_cast_region. A special case is needed for when PARENT is of
> type
> array_type whose type matches TYPE. In this case, get_offset_region is
> allowed
> to continue to normal conclusion.
>
> The original bug noted in gcc/testsuite/gcc.dg/analyzer/casts-1.c was for
> the
> case:
>
> struct s1
> {
> char a;
> char b;
> char c;
> char d;
> };
>
> struct s2
> {
> char arr[4];
> };
>
> struct s2 x = {{'A', 'B', 'C', 'D'}};
> struct s1 *p = (struct s1 *)
> __analyzer_eval (p->a == 'A');
> //etc
>
> This requires a case added to region_model_manager::maybe_fold_sub_svalue
> in
> the individual characters from string constant section for a field region.
>
> Finally, the prior only works for the case where struct s2 was a single
> field
> struct. The more general case is:
>
> struct s2
> {
> char arr1[2];
> char arr2[2];
> };
>
> struct s2 x = {{'A', 'B'}, {'C', 'D'}};
This is s3 in the testcase, rather than s2; looks like this commit
message should be updated accordingly to match your change to the
testcase.
BTW, did this case arise in practice? The existing cases are rather
artificial; IIRC I created them whilst experimenting with various casts
whilst prototypeing the code, in the hope of generating coverage, but
without any specific real-world examples in mind. (kind of "kicking
the tires", if you will). Am I right in thinking that this new one
arose in a similar way?
> Here the array will never be found in the get_any_binding routines. A new
> routine is added to class binding_cluster that checks if the region being
> searched is a field_region of a cast_region, and if so, tries to find a
> field
> of the original region that contains the region under examination. This
> new
> function is named binding_cluster::get_parent_cast_binding. It is called
> from
> get_binding_recursive.
>
> gcc/analyzer/ChangeLog:
> PR analyzer/98797
> * region-model-manager.cc
> region_model_manager::get_offset_region: Add
> check for a PARENT array whose type matches TYPE, and have this
> skip
> returning get_cast_region and rather conclude the function
> normally.
> * region-model-manager.cc
> region_model_manager::maybe_fold_sub_svalue
> Update the get character from string_cst section to include cases
> for
> an offset_region and a field_region.
> * store.cc binding_cluster::get_binding_recursive: Add case for
> call
> to new function get_parent_cast_binding.
> * store.cc binding_cluster::get_parent_cast_binding: New function.
> * store.h class binding_cluster: Add declaration for new member
> function get_parent_class_binding and macros for bit to byte
> conversion.
Formatting nit: the items in the ChangeLog within each file should be
enclosed by parentheses. We have a git commit hook that verifies the
format. In theory there's a way to run it ahead of time, but I don't
know of the top of my head where it is.
> gcc/testsuite/ChangeLog:
> PR analyzer/98797
> * gcc.dg/analyzer/casts-1.c:
Re: rs6000: Fix invalid splits when using Altivec style addresses [PR98959]
On Fri, Feb 12, 2021 at 02:50:12PM -0600, Peter Bergner wrote:
> The rs6000_emit_le_vsx_* functions assume they are not passed an Altivec
> style "& ~16" address. However, some of our expanders and splitters do
> not verify we do not have an Altivec style address before calling those
> functions, leading to an ICE. The solution here is to guard the expanders
> and splitters to ensure we do not call them if we're given an Altivec style
> address.
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -10059,6 +10059,11 @@ rs6000_const_vec (machine_mode mode)
> void
> rs6000_emit_le_vsx_permute (rtx dest, rtx source, machine_mode mode)
> {
> + if (MEM_P (dest))
> +gcc_assert (!altivec_indexed_or_indirect_operand (dest, mode));
> + if (MEM_P (source))
> +gcc_assert (!altivec_indexed_or_indirect_operand (source, mode));
altivec_indexed_or_indirect_operand returns false if passed something
not a mem, so this is just
gcc_assert (!altivec_indexed_or_indirect_operand (dest, mode));
gcc_assert (!altivec_indexed_or_indirect_operand (source, mode));
Please retest with that tweak. Okay for trunk. Thanks!
Also okay for GCC 10. Do you need backports to earlier? Which then?
Segher
[PATCH] c++: Fix folding of non-dependent BASELINKs [PR95468]
Here, the problem ultimately seems to be that tsubst_copy_and_build,
when called with empty args as we do during non-dependent expression
folding, doesn't touch BASELINKs at all: it delegates to tsubst_copy
which then immediately exits early due to the empty args. This means
that the CAST_EXPR int(1) in the BASELINK A::condition never
gets folded (as part of folding of the overall CALL_EXPR), which later
causes us to crash when performing overload resolution of the rebuilt
CALL_EXPR (which is in terms of this still-templated BASELINK).
This doesn't happen when condition() is a namespace-scope function
because then condition is represented as a TEMPLATE_ID_EXPR
rather than a BASELINK, which does get handled directly from
tsubst_copy_and_build.
This patch fixes this issue by having tsubst_copy_and_build handle
BASELINK directly rather than delegating to tsubst_copy, so that it
processes BASELINKS even when args is empty.
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?
gcc/cp/ChangeLog:
PR c++/95468
* pt.c (tsubst_copy_and_build) : New case, copied
over from tsubst_copy.
gcc/testsuite/ChangeLog:
PR c++/95468
* g++.dg/template/non-dependent15.C: New test.
---
gcc/cp/pt.c | 5 +
gcc/testsuite/g++.dg/template/non-dependent15.C | 12
2 files changed, 17 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/template/non-dependent15.C
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 5102bf02d0f..5b2f43dc5c1 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -19856,6 +19856,11 @@ tsubst_copy_and_build (tree t,
case SCOPE_REF:
RETURN (tsubst_qualified_id (t, args, complain, in_decl, /*done=*/true,
/*address_p=*/false));
+
+case BASELINK:
+ return tsubst_baselink (t, current_nonlambda_class_type (),
+ args, complain, in_decl);
+
case ARRAY_REF:
op1 = tsubst_non_call_postfix_expression (TREE_OPERAND (t, 0),
args, complain, in_decl);
diff --git a/gcc/testsuite/g++.dg/template/non-dependent15.C
b/gcc/testsuite/g++.dg/template/non-dependent15.C
new file mode 100644
index 000..00dfe26d6ba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/non-dependent15.C
@@ -0,0 +1,12 @@
+// PR c++/95468
+// { dg-do compile { target c++11 } }
+
+struct A {
+ template
+ static constexpr int condition() { return N; }
+};
+
+template struct B {};
+
+template
+using T = B()>;
--
2.30.0.452.gfb7fa4a1fd
[PATCH] c++: Micro-optimize instantiation_dependent_expression_p
This makes instantiation_dependent_expression_p avoid checking
potential_constant_expression when processing_template_decl isn't set
(and hence when value_dependent_expression_p is definitely false).
gcc/cp/ChangeLog:
* pt.c (instantiation_dependent_expression_p): Check
processing_template_decl before checking
potential_constant_expression.
---
gcc/cp/pt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index dd89dd6f896..5102bf02d0f 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -27519,7 +27519,8 @@ bool
instantiation_dependent_expression_p (tree expression)
{
return (instantiation_dependent_uneval_expression_p (expression)
- || (potential_constant_expression (expression)
+ || (processing_template_decl
+ && potential_constant_expression (expression)
&& value_dependent_expression_p (expression)));
}
--
2.30.0.452.gfb7fa4a1fd
Re: rtl-optimization: Fix uninitialized use of opaque mode variable ICE [PR98872]
On 2/12/21 4:21 PM, Peter Bergner wrote: > rtl-optimization: Fix uninitialized use of opaque mode variable ICE [PR98872] > > The initialize_uninitialized_regs function emits (set (reg:) (CONST0_RTX)) > for all uninitialized pseudo uses. However, some modes (eg, opaque modes) > may not have a CONST0_RTX defined, leading to an ICE when we try and create > the initialization insn. The fix is to skip emitting the initialization > if there is no CONST0_RTX defined for the mode. > > This following patch fixes the ICE and is currently regtesting. > Ok for trunk if the bootstrap and regtesting come back clean? > > Peter > > > 2021-02-12 Peter Bergner > > gcc/ > PR rtl-optimization/98872 > * init-regs.c (initialize_uninitialized_regs): Skip initialization > if CONST0_RTX is NULL. > > gcc/testsuite/ > PR rtl-optimization/98872 > * gcc.target/powerpc/pr98872.c: New test. Testing came back clean with no regressions. Peter
[PATCH] c++: ICE with deduction guide in checking type-dep [PR99009, PR97034]
We represent deduction guides with FUNCTION_DECLs, but they are built
without DECL_CONTEXT, leading to an ICE in type_dependent_expression_p
on the assert that the type of a function template with no dependent
(innermost!) template arguments must be non-dependent. Consider the
attached class-deduction79.C: we create a deduction guide:
template G(T)-> E::G
we deduce T and create a partial instantiation:
G(T) -> E::G [with T = int]
And then do_class_deduction wants to create a CALL_EXPR from the above
using build_new_function_call -> build_over_call which calls mark_used
-> maybe_instantiate_noexcept -> type_dependent_expression_p.
There, the innermost template arguments are non-dependent (), but
the fntype is dependent -- the return type is a TYPENAME_TYPE, and
since we have no DECL_CONTEXT, this check holds:
/* Otherwise, if the function decl isn't from a dependent scope, it can't be
type-dependent. Checking this is important for functions with auto return
type, which looks like a dependent type. */
if (TREE_CODE (expression) == FUNCTION_DECL
&& !(DECL_CLASS_SCOPE_P (expression)
&& dependent_type_p (DECL_CONTEXT (expression)))
whereupon we ICE.
Experiments with setting DECL_CONTEXT didn't pan out. So perhaps we
just want to skip the assert for deduction guides, because they are
a little special. Better ideas solicited.
Bootstrapped/regtested on x86_64-pc-linux-gnu.
gcc/cp/ChangeLog:
PR c++/97034
PR c++/99009
* pt.c (type_dependent_expression_p): Don't assert that the type
of a deduction guide must non-dependent.
gcc/testsuite/ChangeLog:
PR c++/97034
PR c++/99009
* g++.dg/cpp1z/class-deduction79.C: New test.
* g++.dg/cpp1z/class-deduction80.C: New test.
* g++.dg/cpp2a/class-deduction-aggr8.C: New test.
* g++.dg/cpp2a/class-deduction-aggr9.C: New test.
---
gcc/cp/pt.c | 5 -
.../g++.dg/cpp1z/class-deduction79.C | 20 +++
.../g++.dg/cpp1z/class-deduction80.C | 12 +++
.../g++.dg/cpp2a/class-deduction-aggr8.C | 19 ++
.../g++.dg/cpp2a/class-deduction-aggr9.C | 18 +
5 files changed, 73 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1z/class-deduction79.C
create mode 100644 gcc/testsuite/g++.dg/cpp1z/class-deduction80.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr8.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr9.C
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 60de8e93ff7..3e6f3407d40 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -27282,7 +27282,10 @@ type_dependent_expression_p (tree expression)
&& DECL_UNIQUE_FRIEND_P (expression)
&& (!DECL_FRIEND_CONTEXT (expression)
|| dependent_type_p (DECL_FRIEND_CONTEXT (expression
- && !DECL_LOCAL_DECL_P (expression))
+ && !DECL_LOCAL_DECL_P (expression)
+ /* We build deduction guides without any DECL_CONTEXT, but they can
+be type-dependent. */
+ && !deduction_guide_p (expression))
{
gcc_assert (!dependent_type_p (TREE_TYPE (expression))
|| undeduced_auto_decl (expression));
diff --git a/gcc/testsuite/g++.dg/cpp1z/class-deduction79.C
b/gcc/testsuite/g++.dg/cpp1z/class-deduction79.C
new file mode 100644
index 000..86a68248157
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/class-deduction79.C
@@ -0,0 +1,20 @@
+// PR c++/97034
+// { dg-do compile { target c++17 } }
+
+template
+struct E {
+ template
+ struct G {
+T t;
+G(T) { }
+ };
+
+ void fn() { G{1}; }
+};
+
+void
+g ()
+{
+ E e;
+ e.fn ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp1z/class-deduction80.C
b/gcc/testsuite/g++.dg/cpp1z/class-deduction80.C
new file mode 100644
index 000..238024c508f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/class-deduction80.C
@@ -0,0 +1,12 @@
+// PR c++/99009
+// { dg-do compile { target c++17 } }
+
+template struct B {
+ B(int = A()) {}
+ template struct A;
+};
+
+template struct X {
+ template struct Y;
+ X() { Y y; };
+};
diff --git a/gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr8.C
b/gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr8.C
new file mode 100644
index 000..399061100ae
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr8.C
@@ -0,0 +1,19 @@
+// PR c++/97034
+// { dg-do compile { target c++20 } }
+
+namespace N {
+template struct S {
+ template S(T, U);
+};
+} // namespace N
+template struct E {
+ template struct G { T t; };
+ void fn() { G{N::S{'a', 1}}; }
+};
+
+void
+g ()
+{
+ E<1> e;
+ e.fn ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr9.C
b/gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr9.C
new file mode 100644
index 000..245a04cd5f9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr9.C
@@ -0,0 +1,18 @@
+// PR c++/97034
+// {
Re: [PATCH] df: Record all definitions in DF_LR_BB_INFO->def [PR98863]
On Thu, Feb 11, 2021 at 03:03:38PM +, Richard Sandiford via Gcc-patches
wrote:
> gcc/
> * df-problems.c (df_lr_bb_local_compute): Treat partial definitions
> as read-modify operations.
>
> gcc/testsuite/
> * gcc.dg/rtl/aarch64/multi-subreg-1.c: New test.
The test fails everywhere but on aarch64.
Fixed thusly, tested on x86_64-linux -m32/-m64, committed to trunk as
obvious:
2021-02-13 Jakub Jelinek
* gcc.dg/rtl/aarch64/multi-subreg-1.c: Add dg-do compile directive
and restrict the test to aarch64-*-* target only.
--- gcc/testsuite/gcc.dg/rtl/aarch64/multi-subreg-1.c.jj2021-02-12
23:57:30.594140834 +0100
+++ gcc/testsuite/gcc.dg/rtl/aarch64/multi-subreg-1.c 2021-02-13
00:01:04.935749889 +0100
@@ -1,3 +1,4 @@
+/* { dg-do compile { target aarch64-*-* } } */
/* { dg-additional-options "-O -fdump-rtl-cse1-all" } */
__int128 __RTL (startwith ("vregs")) foo (void)
Jakub
rtl-optimization: Fix uninitialized use of opaque mode variable ICE [PR98872]
On 2/8/21 7:29 AM, Segher Boessenkool wrote:
>> I think we should either add a new rtx code for constant opaque modes
>> or make init-regs just emit the clobber for opaque modes (and not emit
>> the move).
>
> Thanks for looking Richard. That last option sounds good to me as well.
Ok, guarding the emit_move_insn with a CONST0_RTX test which causes us
to only emit the clobber for opaque modes fixes the problem. Nice!
That means we can eliminate the rest of the patch other than the test case!
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr98872.c
>>> @@ -0,0 +1,20 @@
>>> +/* PR target/98872 */
>>> +/* { dg-do compile } */
>>> +/* { dg-require-effective-target power10_ok } */
>>> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
>>> +
>>> +/* Verify we do not ICE on the tests below. */
>
> Do the existing tests already check the expected code for this?
Yes, our mma-builtin-*.c tests check for expected output.
The updated patch below fixes the bug too and is much simpler! :-)
rtl-optimization: Fix uninitialized use of opaque mode variable ICE [PR98872]
The initialize_uninitialized_regs function emits (set (reg:) (CONST0_RTX))
for all uninitialized pseudo uses. However, some modes (eg, opaque modes)
may not have a CONST0_RTX defined, leading to an ICE when we try and create
the initialization insn. The fix is to skip emitting the initialization
if there is no CONST0_RTX defined for the mode.
This following patch fixes the ICE and is currently regtesting.
Ok for trunk if the bootstrap and regtesting come back clean?
Peter
2021-02-12 Peter Bergner
gcc/
PR rtl-optimization/98872
* init-regs.c (initialize_uninitialized_regs): Skip initialization
if CONST0_RTX is NULL.
gcc/testsuite/
PR rtl-optimization/98872
* gcc.target/powerpc/pr98872.c: New test.
diff --git a/gcc/init-regs.c b/gcc/init-regs.c
index 903c6541f10..72e898f3e33 100644
--- a/gcc/init-regs.c
+++ b/gcc/init-regs.c
@@ -105,7 +105,10 @@ initialize_uninitialized_regs (void)
start_sequence ();
emit_clobber (reg);
- emit_move_insn (reg, CONST0_RTX (GET_MODE (reg)));
+ /* PR98872: Only emit an initialization if MODE has a
+CONST0_RTX defined. */
+ if (CONST0_RTX (GET_MODE (reg)))
+ emit_move_insn (reg, CONST0_RTX (GET_MODE (reg)));
move_insn = get_insns ();
end_sequence ();
emit_insn_before (move_insn, insn);
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98872.c
b/gcc/testsuite/gcc.target/powerpc/pr98872.c
new file mode 100644
index 000..f33ad9b48b6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98872.c
@@ -0,0 +1,19 @@
+/* PR target/98872 */
+/* { dg-require-effective-target power10_ok } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
+
+/* Verify we do not ICE on the following tests. */
+
+void
+foo (__vector_quad *dst)
+{
+ __vector_quad acc;
+ *dst = acc;
+}
+
+void
+bar (__vector_pair *dst)
+{
+ __vector_pair pair;
+ *dst = pair;
+}
c++: Seed imported bindings [PR 99039]
As mentioned in 99040's fix, we can get inter-module using decls. If
the using decl is the only reference to an import, we'll have failed to
seed our imports leading to an assertion failure. The fix is
straight-forwards, check binding contents when seeding imports.
gcc/cp/
* module.cc (module_state::write_cluster): Check bindings for
imported using-decls.
gcc/testsuite/
* g++.dg/modules/pr99039_a.C: New.
* g++.dg/modules/pr99039_b.C: New.
--
Nathan Sidwell
diff --git c/gcc/cp/module.cc w/gcc/cp/module.cc
index 37ccddc74a5..520494f2543 100644
--- c/gcc/cp/module.cc
+++ w/gcc/cp/module.cc
@@ -3108,7 +3108,8 @@ private:
unsigned section;
#if CHECKING_P
int importedness; /* Checker that imports not occurring
- inappropriately. */
+ inappropriately. +ve imports ok,
+ -ve imports not ok. */
#endif
public:
@@ -14632,13 +14633,36 @@ module_state::write_cluster (elf_out *to, depset *scc[], unsigned size,
{
depset *dep = b->deps[jx];
- if (!dep->is_binding ()
- && dep->is_import () && !TREE_VISITED (dep->get_entity ()))
+ if (dep->is_binding ())
{
- tree import = dep->get_entity ();
+ /* A cross-module using decl could be here. */
+ for (unsigned ix = dep->deps.length (); --ix;)
+ {
+ depset *bind = dep->deps[ix];
+ if (bind->get_entity_kind () == depset::EK_USING
+ && bind->deps[1]->is_import ())
+ {
+ tree import = bind->deps[1]->get_entity ();
+ if (!TREE_VISITED (import))
+ {
+ sec.tree_node (import);
+ dump (dumper::CLUSTER)
+ && dump ("Seeded import %N", import);
+ }
+ }
+ }
+ /* Also check the namespace itself. */
+ dep = dep->deps[0];
+ }
- sec.tree_node (import);
- dump (dumper::CLUSTER) && dump ("Seeded import %N", import);
+ if (dep->is_import ())
+ {
+ tree import = dep->get_entity ();
+ if (!TREE_VISITED (import))
+ {
+ sec.tree_node (import);
+ dump (dumper::CLUSTER) && dump ("Seeded import %N", import);
+ }
}
}
}
diff --git c/gcc/testsuite/g++.dg/modules/pr99039_a.C w/gcc/testsuite/g++.dg/modules/pr99039_a.C
new file mode 100644
index 000..56041e948db
--- /dev/null
+++ w/gcc/testsuite/g++.dg/modules/pr99039_a.C
@@ -0,0 +1,9 @@
+// PR c++/99039
+// { dg-additional-options -fmodules-ts }
+export module format;
+// { dg-module-cmi format }
+
+export namespace NS
+{
+void Format ();
+}
diff --git c/gcc/testsuite/g++.dg/modules/pr99039_b.C w/gcc/testsuite/g++.dg/modules/pr99039_b.C
new file mode 100644
index 000..6fb76f584fa
--- /dev/null
+++ w/gcc/testsuite/g++.dg/modules/pr99039_b.C
@@ -0,0 +1,9 @@
+// { dg-additional-options -fmodules-ts }
+export module hello;
+// { dg-module-cmi hello }
+import format;
+
+export namespace NS
+{
+using NS::Format;
+}
c++: Register streamed-in decls when new [PR 99040]
With modules one can have using-decls refering to their own scope. This
is the way to export things from the GMF or from an import. The problem
was I was using current_ns == CP_DECL_CONTEXT (decl) to determine
whether a decl should be registered in a namespace level or not. But
that's an inadequate check and we ended up reregistering decls and
creating a circular list. We should be registering the decl when first
encountered -- whether we bind it is orthogonal to that.
PR c++/99040
gcc/cp/
* module.cc (trees_in::decl_value): Call
add_module_namespace_decl
for new namespace-scope entities.
(module_state::read_cluster): Don't call add_module_decl here.
* name-lookup.h (add_module_decl): Rename to ...
(add_module_namespace_decl): ... this.
* name-lookup.c (newbinding_bookkeeping): Move into ...
(do_pushdecl): ... here. Its only remaining caller.
(add_module_decl): Rename to ...
(add_module_namespace_decl): ... here. Add checking-assert for
circularity. Don't call newbinding_bookkeeping, just extern_c
checking and incomplete var checking.
gcc/testsuite/
* g++.dg/modules/pr99040_a.C: New.
* g++.dg/modules/pr99040_b.C: New.
* g++.dg/modules/pr99040_c.C: New.
* g++.dg/modules/pr99040_d.C: New.
--
Nathan Sidwell
diff --git c/gcc/cp/module.cc w/gcc/cp/module.cc
index 0749db8fe94..37ccddc74a5 100644
--- c/gcc/cp/module.cc
+++ w/gcc/cp/module.cc
@@ -8162,6 +8162,12 @@ trees_in::decl_value ()
/* Set the TEMPLATE_DECL's type. */
TREE_TYPE (decl) = TREE_TYPE (inner);
+ if (NAMESPACE_SCOPE_P (decl)
+ && (mk == MK_named || mk == MK_unique
+ || mk == MK_enum || mk == MK_friend_spec)
+ && !(VAR_OR_FUNCTION_DECL_P (decl) && DECL_LOCAL_DECL_P (decl)))
+ add_module_namespace_decl (CP_DECL_CONTEXT (decl), decl);
+
/* The late insertion of an alias here or an implicit member
(next block), is ok, because we ensured that all imports were
loaded up before we started this cluster. Thus an insertion
@@ -14893,20 +14899,6 @@ module_state::read_cluster (unsigned snum)
: 0,
decls, type, visible))
sec.set_overrun ();
-
- if (type
- && CP_DECL_CONTEXT (type) == ns
- && !sec.is_duplicate (type))
- add_module_decl (ns, name, type);
-
- for (ovl_iterator iter (decls); iter; ++iter)
- if (!iter.using_p ())
- {
- tree decl = *iter;
- if (CP_DECL_CONTEXT (decl) == ns
- && !sec.is_duplicate (decl))
- add_module_decl (ns, name, decl);
- }
}
break;
diff --git c/gcc/cp/name-lookup.c w/gcc/cp/name-lookup.c
index 8aa490db634..5aa206d40d4 100644
--- c/gcc/cp/name-lookup.c
+++ w/gcc/cp/name-lookup.c
@@ -382,7 +382,8 @@ add_decl_to_level (cp_binding_level *b, tree decl)
/* Make sure we don't create a circular list. xref_tag can end
up pushing the same artificial decl more than once. We
- should have already detected that in update_binding. */
+ should have already detected that in update_binding. (This isn't a
+ complete verification of non-circularity.) */
gcc_assert (b->names != decl);
/* We build up the list in reverse order, and reverse it later if
@@ -3496,41 +3497,6 @@ implicitly_export_namespace (tree ns)
}
}
-/* DECL has just been bound at LEVEL. finish up the bookkeeping. */
-
-static void
-newbinding_bookkeeping (tree name, tree decl, cp_binding_level *level)
-{
- if (TREE_CODE (decl) == TYPE_DECL)
-{
- tree type = TREE_TYPE (decl);
-
- if (type != error_mark_node)
- {
- if (TYPE_NAME (type) != decl)
- set_underlying_type (decl);
-
- set_identifier_type_value_with_scope (name, decl, level);
-
- if (level->kind != sk_namespace
- && !instantiating_current_function_p ())
- /* This is a locally defined typedef in a function that
- is not a template instantation, record it to implement
- -Wunused-local-typedefs. */
- record_locally_defined_typedef (decl);
- }
-}
- else
-{
- if (VAR_P (decl) && !DECL_LOCAL_DECL_P (decl))
- maybe_register_incomplete_var (decl);
-
- if (VAR_OR_FUNCTION_DECL_P (decl)
- && DECL_EXTERN_C_P (decl))
- check_extern_c_conflict (decl);
-}
-}
-
/* DECL is a global or module-purview entity. If it has non-internal
linkage, and we have a module vector, record it in the appropriate
slot. We have already checked for duplicates. */
@@ -3839,12 +3805,38 @@ do_pushdecl (tree decl, bool hiding)
decl = old;
else
{
- newbinding_bookkeeping (name, decl, level);
+ if (TREE_CODE (decl) == TYPE_DECL)
+ {
+ tree type = TREE_TYPE (decl);
+
+ if (type != error_mark_node)
+ {
+ if (TYPE_NAME (type) != decl)
+ set_underlying_type (decl);
- if (VAR_OR_FUNCTION_DECL_P (decl)
- && DECL_LOCAL_DECL_P (decl)
-
Expunge namespace-scope IDENTIFIER_TYPE_VALUE & global_type_name [PR 99039]
IDENTIFIER_TYPE_VALUE and friends is a remnant of G++'s C origins. It
holds elaborated types on identifier-nodes. While this is fine for C
and for local and class-scopes in C++, it fails badly for namespaces. In
that case a marker 'global_type_node' was used, which essentially
signified 'this is a namespace-scope type *somewhere*', and you'd have
to do a regular name_lookup to find it. As the parser and substitution
machinery has avanced over the last 25 years or so, there's not much
outside of actual name-lookup that uses that. Amusingly the
IDENTIFIER_HAS_TYPE_VALUE predicate will do an actual name-lookup and
then users would repeat that lookup to find the now-known to be there type.
Rather late I realized that this interferes with the lazy loading of
module entities, because we were setting IDENTIFIER_TYPE_VALUE to
global_type_node. But we could be inside some local scope where that
identifier is bound to some local type. Not good!
Rather than add more cruft to look at an identifier's shadow stack and
alter that as necessary, this takes the approach of removing the
existing cruft.
We nuke the few places outside of name lookup that use
IDENTIFIER_TYPE_VALUE. Replacing them with either proper name lookups,
alternative sequences, or in some cases asserting that they (no longer)
happen. Class template instantiation was calling pushtag after setting
IDENTIFIER_TYPE_VALUE in order to stop pushtag creating an implicit
typedef and pushing it, but to get the bookkeeping it needed. Let's
just do the bookkeeping directly.
Then we can stop having a 'bound at namespace-scope' marker at all,
which means lazy loading won't screw up local shadow stacks. Also, it
simplifies set_identifier_type_value_with_scope, as it never needs to
inspect the scope stack. When developing this patch, I discovered a
number of places we'd put an actual namespace-scope type on the
type_value slot, rather than global_type_node. You might notice this is
killing at least two 'why are we doing this?' comments.
While this doesn't fix the two PRs mentioned, it is a necessary step.
PR c++/99039
PR c++/99040
gcc/cp/
* cp-tree.h (CPTI_GLOBAL_TYPE): Delete.
(global_type_node): Delete.
(IDENTIFIER_TYPE_VALUE): Delete.
(IDENTIFIER_HAS_TYPE_VALUE): Delete.
(get_type_value): Delete.
* name-lookup.h (identifier_type_value): Delete.
* name-lookup.c (check_module_override): Don't
SET_IDENTIFIER_TYPE_VALUE here.
(do_pushdecl): Nor here.
(identifier_type_value_1, identifier_type_value): Delete.
(set_identifier_type_value_with_scope): Only
SET_IDENTIFIER_TYPE_VALUE for local and class scopes.
(pushdecl_nanmespace_level): Remove shadow stack nadgering.
(do_pushtag): Use REAL_IDENTIFIER_TYPE_VALUE.
* call.c (check_dtor_name): Use lookup_name.
* decl.c (cxx_init_decl_processing): Drop global_type_node.
* decl2.c (cplus_decl_attributes): Don't
SET_IDENTIFIER_TYPE_VALUE
here.
* init.c (get_type_value): Delete.
* pt.c (instantiate_class_template_1): Don't call pushtag or
SET_IDENTIFIER_TYPE_VALUE here.
(tsubst): Assert never an identifier.
(dependent_type_p): Drop global_type_node assert.
* typeck.c (error_args_num): Don't use
IDENTIFIER_HAS_TYPE_VALUE
to determine ctorness.
gcc/testsuite/
* g++.dg/lookup/pr99039.C: New.
--
Nathan Sidwell
diff --git c/gcc/cp/call.c w/gcc/cp/call.c
index 4744c9768ec..186feef6fe3 100644
--- c/gcc/cp/call.c
+++ w/gcc/cp/call.c
@@ -236,8 +236,15 @@ check_dtor_name (tree basetype, tree name)
|| TREE_CODE (basetype) == ENUMERAL_TYPE)
&& name == constructor_name (basetype))
return true;
- else
- name = get_type_value (name);
+
+ /* Otherwise lookup the name, it could be an unrelated typedef
+ of the correct type. */
+ name = lookup_name (name, LOOK_want::TYPE);
+ if (!name)
+ return false;
+ name = TREE_TYPE (name);
+ if (name == error_mark_node)
+ return false;
}
else
{
@@ -252,8 +259,6 @@ check_dtor_name (tree basetype, tree name)
return false;
}
- if (!name || name == error_mark_node)
-return false;
return same_type_p (TYPE_MAIN_VARIANT (basetype), TYPE_MAIN_VARIANT (name));
}
diff --git c/gcc/cp/cp-tree.h w/gcc/cp/cp-tree.h
index 4ed3936ade2..38b31e3908f 100644
--- c/gcc/cp/cp-tree.h
+++ w/gcc/cp/cp-tree.h
@@ -129,7 +129,6 @@ enum cp_tree_index
CPTI_VTBL_TYPE,
CPTI_VTBL_PTR_TYPE,
CPTI_GLOBAL,
-CPTI_GLOBAL_TYPE,
CPTI_ABORT_FNDECL,
CPTI_AGGR_TAG,
CPTI_CONV_OP_MARKER,
@@ -250,7 +249,6 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
#define std_node cp_global_trees[CPTI_STD]
#define abi_node
Re: Merge from trunk to gccgo branch
I merged trunk revision 9769564e7456453e2273071d0faa5aab2554ff78 to the gccgo branch. Ian
rs6000: Fix invalid splits when using Altivec style addresses [PR98959]
The rs6000_emit_le_vsx_* functions assume they are not passed an Altivec
style "& ~16" address. However, some of our expanders and splitters do
not verify we do not have an Altivec style address before calling those
functions, leading to an ICE. The solution here is to guard the expanders
and splitters to ensure we do not call them if we're given an Altivec style
address.
This fixes the ICE. Ok for mainline if my powerpc64le-linux regtesting
comes back clean?
We'll want backports once this has time to bake on mainline for a while.
Ok there too assuming my regtests there are clean?
Peter
2021-02-12 Peter Bergner
gcc/
PR target/98959
* config/rs6000/rs6000.c (rs6000_emit_le_vsx_permute): Add an assert
to ensure we do not have an Altivec style address.
* config/rs6000/vsx.md (*vsx_le_perm_load_): Disable if passed
an Altivec style address.
(*vsx_le_perm_store_): Likewise.
(splitters after *vsx_le_perm_store_): Likewise.
(vsx_load_): Disable special expander if passed an Altivec
style address.
(vsx_store_): Likewise.
gcc/testsuite/
PR target/98959
* gcc.target/powerpc/pr98959.c: New test.
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..e147cbdb52f 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -10059,6 +10059,11 @@ rs6000_const_vec (machine_mode mode)
void
rs6000_emit_le_vsx_permute (rtx dest, rtx source, machine_mode mode)
{
+ if (MEM_P (dest))
+gcc_assert (!altivec_indexed_or_indirect_operand (dest, mode));
+ if (MEM_P (source))
+gcc_assert (!altivec_indexed_or_indirect_operand (source, mode));
+
/* Scalar permutations are easier to express in integer modes rather than
floating-point modes, so cast them here. We use V1TImode instead
of TImode to ensure that the values don't go through GPRs. */
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index 3e0518631df..f6fe88d3600 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -987,11 +987,13 @@ (define_insn_and_split "*vsx_le_undo_permute_"
(define_insn_and_split "*vsx_le_perm_load_"
[(set (match_operand:VSX_LE_128 0 "vsx_register_operand" "=wa,r")
(match_operand:VSX_LE_128 1 "memory_operand" "Z,Q"))]
- "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR"
+ "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR
+ && !altivec_indexed_or_indirect_operand (operands[1], mode)"
"@
#
#"
- "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR"
+ "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR
+ && !altivec_indexed_or_indirect_operand (operands[1], mode)"
[(const_int 0)]
{
rtx tmp = (can_create_pseudo_p ()
@@ -1008,7 +1010,8 @@ (define_insn_and_split "*vsx_le_perm_load_"
(define_insn "*vsx_le_perm_store_"
[(set (match_operand:VSX_LE_128 0 "memory_operand" "=Z,Q")
(match_operand:VSX_LE_128 1 "vsx_register_operand" "+wa,r"))]
- "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR"
+ "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR
+ & !altivec_indexed_or_indirect_operand (operands[0], mode)"
"@
#
#"
@@ -1019,7 +1022,8 @@ (define_insn "*vsx_le_perm_store_"
(define_split
[(set (match_operand:VSX_LE_128 0 "memory_operand")
(match_operand:VSX_LE_128 1 "vsx_register_operand"))]
- "!BYTES_BIG_ENDIAN && TARGET_VSX && !reload_completed && !TARGET_P9_VECTOR"
+ "!BYTES_BIG_ENDIAN && TARGET_VSX && !reload_completed && !TARGET_P9_VECTOR
+ && !altivec_indexed_or_indirect_operand (operands[0], mode)"
[(const_int 0)]
{
rtx tmp = (can_create_pseudo_p ()
@@ -1075,7 +1079,8 @@ (define_peephole2
(define_split
[(set (match_operand:VSX_LE_128 0 "memory_operand")
(match_operand:VSX_LE_128 1 "vsx_register_operand"))]
- "!BYTES_BIG_ENDIAN && TARGET_VSX && reload_completed && !TARGET_P9_VECTOR"
+ "!BYTES_BIG_ENDIAN && TARGET_VSX && reload_completed && !TARGET_P9_VECTOR
+ && !altivec_indexed_or_indirect_operand (operands[0], mode)"
[(const_int 0)]
{
rs6000_emit_le_vsx_permute (operands[1], operands[1], mode);
@@ -1241,7 +1246,8 @@ (define_expand "vsx_load_"
"VECTOR_MEM_VSX_P (mode)"
{
/* Expand to swaps if needed, prior to swap optimization. */
- if (!BYTES_BIG_ENDIAN && !TARGET_P9_VECTOR)
+ if (!BYTES_BIG_ENDIAN && !TARGET_P9_VECTOR
+ && !altivec_indexed_or_indirect_operand(operands[1], mode))
{
rs6000_emit_le_vsx_move (operands[0], operands[1], mode);
DONE;
@@ -1254,7 +1260,8 @@ (define_expand "vsx_store_"
"VECTOR_MEM_VSX_P (mode)"
{
/* Expand to swaps if needed, prior to swap optimization. */
- if (!BYTES_BIG_ENDIAN && !TARGET_P9_VECTOR)
+ if (!BYTES_BIG_ENDIAN && !TARGET_P9_VECTOR
+ && !altivec_indexed_or_indirect_operand(operands[0], mode))
{
rs6000_emit_le_vsx_move (operands[0], operands[1], mode);
DONE;
diff --git
Re: [PATCH] plug memory leaks in warn_parm_array_mismatch (PR 99055)
On 2/12/21 12:36 PM, Richard Biener wrote:
On February 12, 2021 7:21:25 PM GMT+01:00, Martin Sebor
wrote:
On 2/12/21 12:35 AM, Richard Biener wrote:
On Thu, Feb 11, 2021 at 7:35 PM Martin Sebor
wrote:
On 2/11/21 12:59 AM, Richard Biener wrote:
On Wed, Feb 10, 2021 at 6:16 PM Martin Sebor
wrote:
The attached patch replaces calls to print_generic_expr_to_str()
with
a helper function that returns a std::string and releases the
caller
from the responsibility to explicitly free memory.
I don't like this. What's the reason to use generic_expr_as_string
anyway? We have %E pretty-printers after all.
[Reposting; my first reply was too big.]
The VLA bound is either an expression or the asterisk (*) when
newbnd
is null. The reason for using the function is to avoid duplicating
the warning call and making one with %E and another with "*".
Couldn't you have
fixed the leak by doing
if (newbnd)
free (newbndstr);
etc.?
Yes, I could have. I considered it and chose not to because this
is much simpler, safer (if newbnd were to change after newbndstr
is set), and more in the "C++ spirit" (RAII). This code already
uses std::string (attr_access::array_as_string) and so my change
is in line with it.
I understand that you prefer a more C-ish coding style so if you
consider it a prerequisite for accepting this fix I can make
the change conform to it. See the attached revision (although
I hope you'll see why I chose not to go this route).
I can understand but I still disagree ;)
For what it's worth, print_generic_expr_to_str() would be improved
by returning std::string. It would mean including in
most sources in the compiler, which I suspect may not be a popular
suggestion with everyone here, and which is why I didn't make it
to begin with. But if you're open to it I'm happy to make that
change either for GCC 12 or even now.
Well, looking at print_generic_expr_to_str I see
/* Print a single expression T to string, and return it. */
char *
print_generic_expr_to_str (tree t)
{
pretty_printer pp;
dump_generic_node (, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
return xstrdup (pp_formatted_text ());
}
which makes me think that this instead should be sth like
class generic_expr_as_str : public pretty_printer
{
generic_expr_as_str (tree t) { dump_generic_node (, t, 0,
TDF_VOPS|TDF_MEMSYMS, false); }
operator const char *() { return pp_formatted_text (); }
pretty_printer pp;
};
This wouldn't be a good general solution either (in fact, I'd say
it would make it worse) because the string's lifetime is tied to
the lifetime of the class object, turning memory leaks to uses-
after-free. Consider:
const char *str = generic_expr_as_str (node);
...
warning ("node = %s", str); // str is dangling/invalid
(Trying to "fix" it by replacing the conversion operator with a named
member function like str() doesn't solve the problem.)
But the issue with std::string is that people will have to use .c_str() because
of the gazillion C style interfaces in GCC
and storage of std::string will eventually lead to lots of copying or use the
other very same use after free or leak issues.
std::string isn't the great solution you are presenting it as.
It's the standard solution, but it isn't the only one. If we don't
want to use it we can create our own, improved class (I did mention
auto_vec). I'm only advocating the use of RAII to avoid these hunts
for memory leaks that can be trivially avoided by adopting basic C++
idioms.
Martin
Richard.
As we do seem to have a RAII pretty-printer class already. That
said,
I won't mind using
pretty_printer pp;
dump_generic_node (, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
and pp_formatted_text () in the users either.
Okay.
That is, print_generic_expr_to_str looks just like a bad designed
hack.
Using std::string there doesn't make it any better.
Don't you agree to that?
I agree that print_generic_expr_to_str() could be improved. But
a simple API that returns a string representation of a tree node
needs to be easy to use safely and correctly and hard to misuse.
It shouldn't require its clients to explicitly manage the lifetimes
of multiple objects.
But this isn't a new problem, and the solution is as old as C++
itself: have the API return an object of an RAII class like
std::string, or more generally something like std::unique_ptr.
In this case it could even be GCC's auto_vec, or (less
user-friendly) a garbage collected STRING_CST.
So your 2nd variant patch is OK but you might consider just using
a pretty-printer manually and even do away with the xstrdup in that
way entirely!
Avoiding the xstrdup sounds good to me. I've changed the code to
use the pretty_printer directly and committed the attached patch.
Martin
With the patch installed, Valgrind shows more leaks in this code
that
I'm not sure what to do about:
1) A tree built by build_type_attribute_qual_variant() called from
Re: [PATCH] plug memory leaks in warn_parm_array_mismatch (PR 99055)
On February 12, 2021 7:21:25 PM GMT+01:00, Martin Sebor
wrote:
>On 2/12/21 12:35 AM, Richard Biener wrote:
>> On Thu, Feb 11, 2021 at 7:35 PM Martin Sebor
>wrote:
>>>
>>> On 2/11/21 12:59 AM, Richard Biener wrote:
On Wed, Feb 10, 2021 at 6:16 PM Martin Sebor
>wrote:
>
> The attached patch replaces calls to print_generic_expr_to_str()
>with
> a helper function that returns a std::string and releases the
>caller
> from the responsibility to explicitly free memory.
I don't like this. What's the reason to use generic_expr_as_string
anyway? We have %E pretty-printers after all.
>>>
>>> [Reposting; my first reply was too big.]
>>>
>>> The VLA bound is either an expression or the asterisk (*) when
>newbnd
>>> is null. The reason for using the function is to avoid duplicating
>>> the warning call and making one with %E and another with "*".
>>>
Couldn't you have
fixed the leak by doing
if (newbnd)
free (newbndstr);
etc.?
>>>
>>> Yes, I could have. I considered it and chose not to because this
>>> is much simpler, safer (if newbnd were to change after newbndstr
>>> is set), and more in the "C++ spirit" (RAII). This code already
>>> uses std::string (attr_access::array_as_string) and so my change
>>> is in line with it.
>>>
>>> I understand that you prefer a more C-ish coding style so if you
>>> consider it a prerequisite for accepting this fix I can make
>>> the change conform to it. See the attached revision (although
>>> I hope you'll see why I chose not to go this route).
>>
>> I can understand but I still disagree ;)
>>
>>> For what it's worth, print_generic_expr_to_str() would be improved
>>> by returning std::string. It would mean including in
>>> most sources in the compiler, which I suspect may not be a popular
>>> suggestion with everyone here, and which is why I didn't make it
>>> to begin with. But if you're open to it I'm happy to make that
>>> change either for GCC 12 or even now.
>>
>> Well, looking at print_generic_expr_to_str I see
>>
>> /* Print a single expression T to string, and return it. */
>>
>> char *
>> print_generic_expr_to_str (tree t)
>> {
>>pretty_printer pp;
>>dump_generic_node (, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
>>return xstrdup (pp_formatted_text ());
>> }
>>
>> which makes me think that this instead should be sth like
>>
>> class generic_expr_as_str : public pretty_printer
>> {
>> generic_expr_as_str (tree t) { dump_generic_node (, t, 0,
>> TDF_VOPS|TDF_MEMSYMS, false); }
>> operator const char *() { return pp_formatted_text (); }
>> pretty_printer pp;
>> };
>
>This wouldn't be a good general solution either (in fact, I'd say
>it would make it worse) because the string's lifetime is tied to
>the lifetime of the class object, turning memory leaks to uses-
>after-free. Consider:
>
> const char *str = generic_expr_as_str (node);
> ...
> warning ("node = %s", str); // str is dangling/invalid
>
>(Trying to "fix" it by replacing the conversion operator with a named
>member function like str() doesn't solve the problem.)
But the issue with std::string is that people will have to use .c_str() because
of the gazillion C style interfaces in GCC
and storage of std::string will eventually lead to lots of copying or use the
other very same use after free or leak issues.
std::string isn't the great solution you are presenting it as.
Richard.
>>
>> As we do seem to have a RAII pretty-printer class already. That
>said,
>> I won't mind using
>>
>> pretty_printer pp;
>> dump_generic_node (, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
>>
>> and pp_formatted_text () in the users either.
>
>Okay.
>
>>
>> That is, print_generic_expr_to_str looks just like a bad designed
>hack.
>> Using std::string there doesn't make it any better.
>>
>> Don't you agree to that?
>
>I agree that print_generic_expr_to_str() could be improved. But
>a simple API that returns a string representation of a tree node
>needs to be easy to use safely and correctly and hard to misuse.
>It shouldn't require its clients to explicitly manage the lifetimes
>of multiple objects.
>
>But this isn't a new problem, and the solution is as old as C++
>itself: have the API return an object of an RAII class like
>std::string, or more generally something like std::unique_ptr.
>In this case it could even be GCC's auto_vec, or (less
>user-friendly) a garbage collected STRING_CST.
>
>>
>> So your 2nd variant patch is OK but you might consider just using
>> a pretty-printer manually and even do away with the xstrdup in that
>> way entirely!
>
>Avoiding the xstrdup sounds good to me. I've changed the code to
>use the pretty_printer directly and committed the attached patch.
>
>Martin
>
>>
> With the patch installed, Valgrind shows more leaks in this code
>that
> I'm not sure what to do about:
>
> 1) A tree built by build_type_attribute_qual_variant() called from
>
Go patch committed: Use correct path for string/[]byte embed
This patch by Michael Matloob fixes the Go frontend to use the correct path when opening an embedded file for a string or []byte type. For the other embed.FS case we were correctly using the Files mapping, but for string or []byte we were not. Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu. Committed to mainline. Ian 7767de2344dca80cc95a982b048c341b72c169c2 diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE index 905c6fe9326..3175ba0e005 100644 --- a/gcc/go/gofrontend/MERGE +++ b/gcc/go/gofrontend/MERGE @@ -1,4 +1,4 @@ -78770fd9c29037dec8b2919c0f02067915c6ad33 +a5d7c4225fbbd06b97db6fa424cc0cb5191082d4 The first line of this file holds the git revision number of the last merge done from the gofrontend repository. diff --git a/gcc/go/gofrontend/embed.cc b/gcc/go/gofrontend/embed.cc index bea1003bc08..0a7df0531ec 100644 --- a/gcc/go/gofrontend/embed.cc +++ b/gcc/go/gofrontend/embed.cc @@ -812,8 +812,7 @@ Gogo::initializer_for_embeds(Type* type, } // Each pattern in the embedcfg file maps to a list of file - // names. For each file name, the embedcfg file records an - // absolute path. Add those absolute paths to PATHS. + // names. Add those file names to PATHS. for (std::vector::const_iterator pf = pp->second.begin(); pf != pp->second.end(); pf++) @@ -865,7 +864,7 @@ Gogo::initializer_for_embeds(Type* type, } std::string data; - if (!read_file(paths[0].c_str(), loc, )) + if (!read_file(this->embed_files_[paths[0]].c_str(), loc, )) return Expression::make_error(loc); Expression* e = Expression::make_string(data, loc);
Re: [PATCH 4/4] c++: dependent constraint on placeholder 'auto' [PR96443]
On 2/11/21 5:14 PM, Patrick Palka wrote: On Thu, 11 Feb 2021, Jason Merrill wrote: On 2/8/21 2:03 PM, Patrick Palka wrote: This fixes the way we check satisfaction of constraints on placeholder types in various contexts, and in particular when the constraint is dependent. Firstly, when evaluating the return type requirement of a compound requirement, we currently substitute the outer template arguments into the constraint before checking satisfaction. But we should instead be passing in the complete set of template arguments to satisfaction and not do a prior separate substitution. Our current approach leads to us incorrectly rejecting the testcase concepts-return-req2.C below. Secondly, when checking the constraints on a placeholder variable or return type, we don't substitute the template arguments of the enclosing context at all. This leads to bogus errors during satisfaction when the constraint is dependent as in the testcase concepts-placeholder3.C below. In order to fix these two issues, we need to be able to properly normalize the constraints on a placeholder 'auto', which in turn requires us to know the template parameters that were in-scope where an 'auto' was introduced. This information currently doesn't seem to be easily available when we need it, so this patch adds an auxiliary hash table that keeps track of the value of current_template_parms when each constrained 'auto' was formed. This patch also removes some seemingly wrong handling of placeholder type arguments from tsubst_parameter_mapping. The code doesn't trigger with the example used in the comments, because type_uses_auto doesn't look inside non-deduced contexts such as the operand of decltype. And the call to do_auto_deduction seems confused because if 'arg' is a type, then so is 'parm', and therefore 'init' too is a type, but do_auto_deduction expects it to be an expression. Before this patch, this code was dead (as far as our testsuite can tell), but now it breaks other parts of this patch, so let's remove it. gcc/cp/ChangeLog: PR c++/96443 * constraint.cc (type_deducible_p): Don't substitute into the constraints, and instead just pass 'args' to do_auto_deduction as the outer template arguments. (tsubst_parameter_mapping): Remove confused code for handling placeholder type arguments. (normalize_placeholder_type_constraint): Define. (satisfy_constraint_expression): Use it to handle placeholder 'auto' types. * cp-tree.h (get_constrained_auto_context): Declare. * pt.c (constrained_auto_context_map): Define. (get_placeholder_type_constraint_context): Define. (set_placeholder_type_constraints): Define. (copy_placeholder_type_constraints): Define. (tsubst) : Use copy_placeholder_type_constraints. (make_constrained_placeholder_type): Use set_placeholder_type_constraints. (do_auto_deduction): Clarify comments about the outer_targs parameter. Rework satisfaction of a placeholder type constraint to pass in the complete set of template arguments directly to constraints_satisfied_p. (splice_late_return_type): Use copy_placeholder_type_constraints. gcc/testsuite/ChangeLog: PR c++/96443 * g++.dg/cpp2a/concepts-placeholder3.C: New test. * g++.dg/cpp2a/concepts-return-req2.C: New test. * g++.dg/concepts-ts1.C: Add dg-bogus directive to the call to f15 that we expect to accept. --- gcc/cp/constraint.cc | 106 -- gcc/cp/cp-tree.h | 1 + gcc/cp/pt.c | 101 +++-- .../g++.dg/cpp2a/concepts-placeholder3.C | 19 .../g++.dg/cpp2a/concepts-return-req2.C | 13 +++ gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +- 6 files changed, 146 insertions(+), 96 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder3.C create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-return-req2.C diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc index 56134f8b2bf..53588047d44 100644 --- a/gcc/cp/constraint.cc +++ b/gcc/cp/constraint.cc @@ -2007,39 +2007,19 @@ type_deducible_p (tree expr, tree type, tree placeholder, tree args, references are preserved in the result. */ expr = force_paren_expr_uneval (expr); - /* Replace the constraints with the instantiated constraints. This - substitutes args into any template parameters in the trailing - result type. */ - tree saved_constr = PLACEHOLDER_TYPE_CONSTRAINTS (placeholder); - tree subst_constr -= tsubst_constraint (saved_constr, -args, -info.complain | tf_partial, -info.in_decl); - - if (subst_constr == error_mark_node) -return false; - - PLACEHOLDER_TYPE_CONSTRAINTS (placeholder) =
Re: [PATCH 3/4] c++: Delay normalizing nested requirements until satisfaction
On 2/10/21 9:41 AM, Patrick Palka wrote:
On Tue, 9 Feb 2021, Jason Merrill wrote:
On 2/8/21 2:03 PM, Patrick Palka wrote:
This sets up the functionality for controlling the initial set of
template parameters to pass to normalization when dealing with a
constraint-expression that is not associated with some constrained
declaration, for instance when normalizing a nested requirement of a
requires expression, or the constraints on a placeholder type.
The main new ingredient here is the data member norm_info::initial_parms
which can be set by callers of the normalization routines to communicate
the in-scope template parameters for the supplied constraint-expression,
rather than always falling back to using current_template_parms.
This patch then uses this functionality in our handling of nested
requirements so that we can delay normalizing them until needed for
satisfaction. We currently immediately normalize nested requirements at
parse time, where we have the necessary template context, and cache the
normal form in their TREE_TYPE node. With this patch, we now delay
normalization until needed (as with other constraint expressions), and
instead store the current value of current_template_parms in their
TREE_TYPE node (which we use to restore the template context at
normalization time).
In the subsequent patch, this functionality will also be used to
normalize placeholder type constraints during auto deduction.
gcc/cp/ChangeLog:
* constraint.cc (build_parameter_mapping): Rely on the caller to
determine the in-scope template parameters.
(norm_info::norm_info): Delegate the one-parameter constructor
to the two-parameter constructor. In the two-parameter
constructor, fold in the definition of make_context, set
initial_parms appropriately, and don't set the now-removed
orig_decl member.
(norm_info::make_context): Remove, now that its only use is
inlined into the caller.
(norm_info::update_context): Adjust call to
build_parameter_mapping to pass in the relevant set of in-scope
template parameters.
(norm_info::ctx_parms): Define this member function.
(norm_info::context): Initialize to NULL_TREE.
(norm_info::orig_decl): Remove this data member.
(norm_info::initial_parms): Define this data member.
(normalize_atom): Adjust call to build_parameter_mapping to pass
in the relevant set of in-scope template parameters. Use
info.initial_parms instead of info.orig_decl.
(normalize_constraint_expression): Define an overload that takes
a norm_info object. Cache the result of normalization. Define
the other overload in terms of this one, and handle a NESTED_REQ
argument by setting info.initial_parms appropriately.
(tsubst_nested_requirement): Go through
satisfy_constraint_expression so that we normalize on demand.
(finish_nested_requirement): Set the TREE_TYPE of the NESTED_REQ
to current_template_parms.
(diagnose_nested_requirements): Go through
satisfy_constraint_expression, as with tsubst_nested_requirement.
---
gcc/cp/constraint.cc | 140 +++
gcc/cp/cp-tree.h | 4 +-
2 files changed, 78 insertions(+), 66 deletions(-)
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 39c97986082..56134f8b2bf 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -133,7 +133,7 @@ struct sat_info : subst_info
bool diagnose_unsatisfaction;
};
-static tree satisfy_constraint (tree, tree, sat_info);
+static tree satisfy_constraint_expression (tree, tree, sat_info);
/* True if T is known to be some type other than bool. Note that this
is false for dependent types and errors. */
@@ -594,26 +594,12 @@ map_arguments (tree parms, tree args)
return parms;
}
-/* Build the parameter mapping for EXPR using ARGS. */
+/* Build the parameter mapping for EXPR using ARGS, where CTX_PARMS
+ are the template parameters in scope for EXPR. */
static tree
-build_parameter_mapping (tree expr, tree args, tree decl)
+build_parameter_mapping (tree expr, tree args, tree ctx_parms)
{
- tree ctx_parms = NULL_TREE;
- if (decl)
-{
- gcc_assert (TREE_CODE (decl) == TEMPLATE_DECL);
- ctx_parms = DECL_TEMPLATE_PARMS (decl);
-}
- else if (current_template_parms)
-{
- /* TODO: This should probably be the only case, but because the
-point of declaration of concepts is currently set after the
-initializer, the template parameter lists are not available
-when normalizing concept definitions, hence the case above. */
- ctx_parms = current_template_parms;
-}
-
tree parms = find_template_parameters (expr, ctx_parms);
tree map = map_arguments (parms, args);
return map;
@@ -645,53 +631,63 @@ parameter_mapping_equivalent_p (tree t1, tree t2)
Re: [PATCH] Fix producer string memory leaks
On 2/12/21 9:56 AM, Martin Sebor wrote:
On 2/12/21 2:09 AM, Richard Biener via Gcc-patches wrote:
On Thu, Feb 11, 2021 at 6:41 PM Martin Liška wrote:
Hello.
This fixes 2 memory leaks I noticed.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
OK.
Thanks,
Martin
gcc/ChangeLog:
* opts-common.c (decode_cmdline_option): Release werror_arg.
* opts.c (gen_producer_string): Release output of
gen_command_line_string.
---
gcc/opts-common.c | 1 +
gcc/opts.c | 7 +--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/gcc/opts-common.c b/gcc/opts-common.c
index 6cb5602896d..5e10edeb4cf 100644
--- a/gcc/opts-common.c
+++ b/gcc/opts-common.c
@@ -766,6 +766,7 @@ decode_cmdline_option (const char *const *argv,
unsigned int lang_mask,
werror_arg[0] = 'W';
size_t warning_index = find_opt (werror_arg, lang_mask);
+ free (werror_arg);
Sorry to butt in here, but since we're having a discussion on this
same subject in another review of a fix for a memory leak, I thought
I'd pipe up: I would suggest a better (more in line with C++ and more
future-proof) solution is to replace the call to xstrdup (and the need
to subsequently call free) with std::string.
if (warning_index != OPT_SPECIAL_unknown)
{
const struct cl_option *warning_option
diff --git a/gcc/opts.c b/gcc/opts.c
index fc5f61e13cc..24bb64198c8 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -3401,8 +3401,11 @@ char *
gen_producer_string (const char *language_string,
cl_decoded_option *options,
unsigned int options_count)
{
- return concat (language_string, " ", version_string, " ",
- gen_command_line_string (options, options_count),
NULL);
+ char *cmdline = gen_command_line_string (options, options_count);
+ char *combined = concat (language_string, " ", version_string, " ",
+ cmdline, NULL);
+ free (cmdline);
+ return combined;
}
Here, changing gen_command_line_string() to return std::string instead
of a raw pointer would similarly avoid having to remember to free
the pointer (and forgetting). The function has two other callers,
both in gcc/toplev.c, and both also leak the memory for the same
reason as here.
It occurs to me that until the APIs are improved (to use RAII), or
where changing them is not feasible, an alternative is to let our
own analyzer detect the leaks by annotating gen_command_line_string
and other functions like it with the new attribute malloc. With
that, this leak is diagnosed like so:
/src/gcc/master/gcc/opts.c: In function ‘char*
gen_command_line_string(cl_decoded_option*, unsigned int)’:
/src/gcc/master/gcc/opts.c:3395:10: warning: leak of ‘cmdline’ [CWE-401]
[-Wanalyzer-malloc-leak]
3395 | return options_string;
| ^~
‘char* gen_producer_string(const char*, cl_decoded_option*, unsigned
int)’: events 1-3
|
| 3401 | gen_producer_string (const char *language_string,
cl_decoded_option *options,
| | ^~~
| | |
| | (1) entry to ‘gen_producer_string’
|..
| 3404 | char *cmdline = gen_command_line_string (options,
options_count);
...
The attribute malloc annotation would look like this:
__attribute__ ((__malloc__, __malloc__ (free)))
extern char *gen_command_line_string (cl_decoded_option *options,
unsigned int options_count);
It would be worthwhile to do a review of GCC's own APIs and add
the attribute wherever appropriate, not just to detect leaks but
other memory management bugs (e.g., calling free on memory
allocated by operator new). Something to consider for GCC 12.
Martin
Re: [PATCH] plug memory leaks in warn_parm_array_mismatch (PR 99055)
On 2/12/21 12:35 AM, Richard Biener wrote:
On Thu, Feb 11, 2021 at 7:35 PM Martin Sebor wrote:
On 2/11/21 12:59 AM, Richard Biener wrote:
On Wed, Feb 10, 2021 at 6:16 PM Martin Sebor wrote:
The attached patch replaces calls to print_generic_expr_to_str() with
a helper function that returns a std::string and releases the caller
from the responsibility to explicitly free memory.
I don't like this. What's the reason to use generic_expr_as_string
anyway? We have %E pretty-printers after all.
[Reposting; my first reply was too big.]
The VLA bound is either an expression or the asterisk (*) when newbnd
is null. The reason for using the function is to avoid duplicating
the warning call and making one with %E and another with "*".
Couldn't you have
fixed the leak by doing
if (newbnd)
free (newbndstr);
etc.?
Yes, I could have. I considered it and chose not to because this
is much simpler, safer (if newbnd were to change after newbndstr
is set), and more in the "C++ spirit" (RAII). This code already
uses std::string (attr_access::array_as_string) and so my change
is in line with it.
I understand that you prefer a more C-ish coding style so if you
consider it a prerequisite for accepting this fix I can make
the change conform to it. See the attached revision (although
I hope you'll see why I chose not to go this route).
I can understand but I still disagree ;)
For what it's worth, print_generic_expr_to_str() would be improved
by returning std::string. It would mean including in
most sources in the compiler, which I suspect may not be a popular
suggestion with everyone here, and which is why I didn't make it
to begin with. But if you're open to it I'm happy to make that
change either for GCC 12 or even now.
Well, looking at print_generic_expr_to_str I see
/* Print a single expression T to string, and return it. */
char *
print_generic_expr_to_str (tree t)
{
pretty_printer pp;
dump_generic_node (, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
return xstrdup (pp_formatted_text ());
}
which makes me think that this instead should be sth like
class generic_expr_as_str : public pretty_printer
{
generic_expr_as_str (tree t) { dump_generic_node (, t, 0,
TDF_VOPS|TDF_MEMSYMS, false); }
operator const char *() { return pp_formatted_text (); }
pretty_printer pp;
};
This wouldn't be a good general solution either (in fact, I'd say
it would make it worse) because the string's lifetime is tied to
the lifetime of the class object, turning memory leaks to uses-
after-free. Consider:
const char *str = generic_expr_as_str (node);
...
warning ("node = %s", str); // str is dangling/invalid
(Trying to "fix" it by replacing the conversion operator with a named
member function like str() doesn't solve the problem.)
As we do seem to have a RAII pretty-printer class already. That said,
I won't mind using
pretty_printer pp;
dump_generic_node (, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
and pp_formatted_text () in the users either.
Okay.
That is, print_generic_expr_to_str looks just like a bad designed hack.
Using std::string there doesn't make it any better.
Don't you agree to that?
I agree that print_generic_expr_to_str() could be improved. But
a simple API that returns a string representation of a tree node
needs to be easy to use safely and correctly and hard to misuse.
It shouldn't require its clients to explicitly manage the lifetimes
of multiple objects.
But this isn't a new problem, and the solution is as old as C++
itself: have the API return an object of an RAII class like
std::string, or more generally something like std::unique_ptr.
In this case it could even be GCC's auto_vec, or (less
user-friendly) a garbage collected STRING_CST.
So your 2nd variant patch is OK but you might consider just using
a pretty-printer manually and even do away with the xstrdup in that
way entirely!
Avoiding the xstrdup sounds good to me. I've changed the code to
use the pretty_printer directly and committed the attached patch.
Martin
With the patch installed, Valgrind shows more leaks in this code that
I'm not sure what to do about:
1) A tree built by build_type_attribute_qual_variant() called from
attr_access::array_as_string() to build a temporary type only
for the purposes of formatting it.
2) A tree (an attribute list) built by tree_cons() called from
build_attr_access_from_parms() that's used only for the duration
of the caller.
Do these temporary trees need to be released somehow or are the leaks
expected?
You should configure GCC with --enable-valgrind-annotations to make
it aware of our GC.
I did configure with that option:
$ /src/gcc/master/configure --enable-checking=yes
--enable-languages=all,jit,lto --enable-host-shared
--enable-valgrind-annotations
Attached to pr99055 is my valgrind output for gcc.dg/Wvla-parameter.c
with the top of trunk and the patch applied:
$ /build/gcc-master/gcc/xgcc -B
Re: [OBVIOUS][PATCH] testsuite, arm: Add -mthumb to pr98931.c [PR target/98931]
On 12/02/2021 17:21, Christophe Lyon wrote:
> On Fri, 12 Feb 2021 at 17:02, Richard Earnshaw
> wrote:
>>
>> On 12/02/2021 14:20, Christophe Lyon via Gcc-patches wrote:
>>> This test forces -march=armv8.1-m.main, which supports only Thumb mode.
>>> However, if the toolchain is not configured --with-thumb, the test
>>> fails with:
>>> error: target CPU does not support ARM mode
>>>
>>> Adding -mthumb to dg-options fixes the problem.
>>>
>>
>> Hmm, this sounds like a different problem. The driver, these days, is
>> supposed to detect when the architecture only supports thumb and to add
>> that automatically when calling cc1 etc. So why isn't that working in
>> this case?
>>
>> (See gcc/common/config/arm/arm-common.c:arm_target_thumb_only).
>>
>
> Well, I've just rebuilt --with-mode=arm --with-cpu=cortex-a9
> --target=arm-none-eabi
> and when I compile
> $ arm-none-eabi-gcc hello.c -march=armv8.1-m.main -S
> cc1: error: target CPU does not support ARM mode
> I put a breakpoint in arm_target_thumb_only and it did not trigger.
Ah, I think it must be the "--with-mode=arm" that causes this. That
must be added before the driver self specs are analysed.
R.
>
>
>> R.
>>
>>> Committed as obvious.
>>>
>>> 2021-02-12 Christophe Lyon
>>>
>>> PR target/98931
>>> gcc/testsuite/
>>> * gcc.target/arm/pr98931.c: Add -mthumb
>>> ---
>>> gcc/testsuite/gcc.target/arm/pr98931.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gcc/testsuite/gcc.target/arm/pr98931.c
>>> b/gcc/testsuite/gcc.target/arm/pr98931.c
>>> index 313876a..66070ad 100644
>>> --- a/gcc/testsuite/gcc.target/arm/pr98931.c
>>> +++ b/gcc/testsuite/gcc.target/arm/pr98931.c
>>> @@ -1,6 +1,6 @@
>>> /* { dg-do assemble } */
>>> /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm"
>>> "-mcpu=*" } } */
>>> -/* { dg-options "-march=armv8.1-m.main -O3
>>> --param=max-completely-peeled-insns=1300 --save-temps" } */
>>> +/* { dg-options "-march=armv8.1-m.main -O3
>>> --param=max-completely-peeled-insns=1300 --save-temps -mthumb" } */
>>>
>>> extern long long a[][20][26][26][22];
>>>
>>>
>>
Re: [OBVIOUS][PATCH] testsuite, arm: Add -mthumb to pr98931.c [PR target/98931]
On Fri, 12 Feb 2021 at 17:02, Richard Earnshaw
wrote:
>
> On 12/02/2021 14:20, Christophe Lyon via Gcc-patches wrote:
> > This test forces -march=armv8.1-m.main, which supports only Thumb mode.
> > However, if the toolchain is not configured --with-thumb, the test
> > fails with:
> > error: target CPU does not support ARM mode
> >
> > Adding -mthumb to dg-options fixes the problem.
> >
>
> Hmm, this sounds like a different problem. The driver, these days, is
> supposed to detect when the architecture only supports thumb and to add
> that automatically when calling cc1 etc. So why isn't that working in
> this case?
>
> (See gcc/common/config/arm/arm-common.c:arm_target_thumb_only).
>
Well, I've just rebuilt --with-mode=arm --with-cpu=cortex-a9
--target=arm-none-eabi
and when I compile
$ arm-none-eabi-gcc hello.c -march=armv8.1-m.main -S
cc1: error: target CPU does not support ARM mode
I put a breakpoint in arm_target_thumb_only and it did not trigger.
> R.
>
> > Committed as obvious.
> >
> > 2021-02-12 Christophe Lyon
> >
> > PR target/98931
> > gcc/testsuite/
> > * gcc.target/arm/pr98931.c: Add -mthumb
> > ---
> > gcc/testsuite/gcc.target/arm/pr98931.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/testsuite/gcc.target/arm/pr98931.c
> > b/gcc/testsuite/gcc.target/arm/pr98931.c
> > index 313876a..66070ad 100644
> > --- a/gcc/testsuite/gcc.target/arm/pr98931.c
> > +++ b/gcc/testsuite/gcc.target/arm/pr98931.c
> > @@ -1,6 +1,6 @@
> > /* { dg-do assemble } */
> > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm"
> > "-mcpu=*" } } */
> > -/* { dg-options "-march=armv8.1-m.main -O3
> > --param=max-completely-peeled-insns=1300 --save-temps" } */
> > +/* { dg-options "-march=armv8.1-m.main -O3
> > --param=max-completely-peeled-insns=1300 --save-temps -mthumb" } */
> >
> > extern long long a[][20][26][26][22];
> >
> >
>
Re: [PATCH] Fix producer string memory leaks
On 2/12/21 2:09 AM, Richard Biener via Gcc-patches wrote:
On Thu, Feb 11, 2021 at 6:41 PM Martin Liška wrote:
Hello.
This fixes 2 memory leaks I noticed.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
OK.
Thanks,
Martin
gcc/ChangeLog:
* opts-common.c (decode_cmdline_option): Release werror_arg.
* opts.c (gen_producer_string): Release output of
gen_command_line_string.
---
gcc/opts-common.c | 1 +
gcc/opts.c| 7 +--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/gcc/opts-common.c b/gcc/opts-common.c
index 6cb5602896d..5e10edeb4cf 100644
--- a/gcc/opts-common.c
+++ b/gcc/opts-common.c
@@ -766,6 +766,7 @@ decode_cmdline_option (const char *const *argv, unsigned
int lang_mask,
werror_arg[0] = 'W';
size_t warning_index = find_opt (werror_arg, lang_mask);
+ free (werror_arg);
Sorry to butt in here, but since we're having a discussion on this
same subject in another review of a fix for a memory leak, I thought
I'd pipe up: I would suggest a better (more in line with C++ and more
future-proof) solution is to replace the call to xstrdup (and the need
to subsequently call free) with std::string.
if (warning_index != OPT_SPECIAL_unknown)
{
const struct cl_option *warning_option
diff --git a/gcc/opts.c b/gcc/opts.c
index fc5f61e13cc..24bb64198c8 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -3401,8 +3401,11 @@ char *
gen_producer_string (const char *language_string, cl_decoded_option *options,
unsigned int options_count)
{
- return concat (language_string, " ", version_string, " ",
-gen_command_line_string (options, options_count), NULL);
+ char *cmdline = gen_command_line_string (options, options_count);
+ char *combined = concat (language_string, " ", version_string, " ",
+ cmdline, NULL);
+ free (cmdline);
+ return combined;
}
Here, changing gen_command_line_string() to return std::string instead
of a raw pointer would similarly avoid having to remember to free
the pointer (and forgetting). The function has two other callers,
both in gcc/toplev.c, and both also leak the memory for the same
reason as here.
Martin
#if CHECKING_P
--
2.30.0
Re: [patch, libfortran] PR95647 operator(.eq.) and operator(==) treated differently
How do I get permissions set so that I can change status of bug reports and assign to myself. My permissions got dissolved during some evolution in the last year. also The master branch has been updated by Jerry DeLisle: https://gcc.gnu.org/g:0631e008adc759cc801d0d034224ee6b4bcf31aa commit r11-7225-g0631e008adc759cc801d0d034224ee6b4bcf31aa Author: Steve Kargl Date: Fri Feb 12 07:58:16 2021 -0800 On 2/11/21 7:02 PM, Jerry DeLisle wrote: The attached patch is another provided from Steve Kargle in the PR report. I have created a test case and regression tested the result. OK for trunk? Regards, Jerry libgfortran: Fix PR95647 by changing the interfaces of operators .eq. and .ne. The FE converts the old school .eq. to ==, and then tracks the ==. The module starts with == and so it does not properly overload the .eq. Reversing the interfaces fixes this. 2021-02-11 Steve Kargl libgfortran/ChangeLog: PR libfortran 95647 * ieee/ieee_arithmetic.F90: Flip interfaces of operators .eq. to == and .ne. to /= . gcc/testsuite/ChangeLog: PR libfortran 95647 * gfortran.dg/ieee/ieee_arithmetic: New test.
[PATCH] rtl-ssa: Reduce the amount of temporary memory needed [PR98863]
The rtl-ssa code uses an on-the-side IL and needs to build that IL
for each block and RTL insn. I'd originally not used the classical
dominance frontier method for placing phis on the basis that it seemed
like more work in this context: we're having to visit everything in
an RPO walk anyway, so for non-backedge cases we can tell immediately
whether a phi node is needed. We then speculatively created phis for
registers that are live across backedges and simplified them later.
This avoided having to walk most of the IL twice (once to build the
initial IL, and once to link uses to phis).
However, as shown in PR98863, this leads to excessive temporary
memory in extreme cases, since we had to record the value of
every live register on exit from every block. In that PR,
there were many registers that were live (but unused) across
a large region of code.
This patch does use the classical approach to placing phis, but tries
to use the existing DF defs information to avoid two walks of the IL.
We still use the previous approach for memory, since there is no
up-front information to indicate whether a block defines memory or not.
However, since memory is just treated as a single unified thing
(like for gimple vops), memory doesn't suffer from the same
scalability problems as registers.
With this change, fwprop no longer seems to be a memory-hog outlier
in the PR: the maximum RSS is similar with and without fwprop.
The PR also shows the problems inherent in using bitmap operations
involving the live-in and live-out sets, which in the testcase are
very large. I've therefore tried to reduce those operations to the
bare minimum.
The patch also includes other compile-time optimisations motivated
by the PR; see the changelog for details.
I tried adding:
for (int i = 0; i < 200; ++i)
{
crtl->ssa = new rtl_ssa::function_info (cfun);
delete crtl->ssa;
}
to fwprop.c to stress the code. fwprop then took 35% of the compile
time for the problematic partition in the PR (measured on a release
build). fwprop takes less than .5% of the compile time when running
normally.
The command:
git diff 0b76990a9d75d97b84014e37519086b81824c307~ gcc/fwprop.c | \
patch -p1 -R
still gives a working compiler that uses the old fwprop.c. The compile
time with that version is very similar.
For a more reasonable testcase like optabs.ii at -O, I saw a 6.7%
compile time regression with the loop above added (i.e. creating
the info 201 times per pass instead of once per pass). That goes
down to 4.8% with -O -g. I can't measure a significant difference
with a normal compiler (no 200-iteration loop).
So I think that (as expected) the patch does make things a bit
slower in the normal case. But like Richi says, peak memory usage
is harder for users to work around than slighter slower compile times.
The patch is going to be a nightmare to review, sorry.
Tested on aarch64-linux-gnu, x86_&4-linux-gnu, armeb-eabi and
powerpc64-linux-gnu. OK to install?
If this seems too risky, I wouldn't mind reverting to the old fwprop.c
for GCC 11 and reinstating the current version for GCC 12. However,
I'd still like to apply some version of the patch for GCC 11 in order
to support aarch64-cc-fusion.cc.
Richard
gcc/
PR rtl-optimization/98863
* rtl-ssa/functions.h (function_info::bb_live_out_info): Delete.
(function_info::build_info): Turn into a declaration, moving the
definition to internals.h.
(function_info::bb_walker): Declare.
(function_info::create_reg_use): Likewise.
(function_info::calculate_potential_phi_regs): Take a build_info
parameter.
(function_info::place_phis, function_info::create_ebbs): Declare.
(function_info::calculate_ebb_live_in_for_debug): Likewise.
(function_info::populate_backedge_phis): Delete.
(function_info::start_block, function_info::end_block): Declare.
(function_info::populate_phi_inputs): Delete.
(function_info::m_potential_phi_regs): Move information to build_info.
* rtl-ssa/internals.h: New file.
(function_info::bb_phi_info): New class.
(function_info::build_info): Moved from functions.h.
Add a constructor and destructor.
(function_info::build_info::ebb_use): Delete.
(function_info::build_info::ebb_def): Likewise.
(function_info::build_info::bb_live_out): Likewise.
(function_info::build_info::tmp_ebb_live_in_for_debug): New variable.
(function_info::build_info::potential_phi_regs): Likewise.
(function_info::build_info::potential_phi_regs_for_debug): Likewise.
(function_info::build_info::ebb_def_regs): Likewise.
(function_info::build_info::bb_phis): Likewise.
(function_info::build_info::bb_mem_live_out): Likewise.
(function_info::build_info::bb_to_rpo): Likewise.
(function_info::build_info::def_stack): Likewise.
Re: [OBVIOUS][PATCH] testsuite, arm: Add -mthumb to pr98931.c [PR target/98931]
On 12/02/2021 14:20, Christophe Lyon via Gcc-patches wrote:
> This test forces -march=armv8.1-m.main, which supports only Thumb mode.
> However, if the toolchain is not configured --with-thumb, the test
> fails with:
> error: target CPU does not support ARM mode
>
> Adding -mthumb to dg-options fixes the problem.
>
Hmm, this sounds like a different problem. The driver, these days, is
supposed to detect when the architecture only supports thumb and to add
that automatically when calling cc1 etc. So why isn't that working in
this case?
(See gcc/common/config/arm/arm-common.c:arm_target_thumb_only).
R.
> Committed as obvious.
>
> 2021-02-12 Christophe Lyon
>
> PR target/98931
> gcc/testsuite/
> * gcc.target/arm/pr98931.c: Add -mthumb
> ---
> gcc/testsuite/gcc.target/arm/pr98931.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/gcc.target/arm/pr98931.c
> b/gcc/testsuite/gcc.target/arm/pr98931.c
> index 313876a..66070ad 100644
> --- a/gcc/testsuite/gcc.target/arm/pr98931.c
> +++ b/gcc/testsuite/gcc.target/arm/pr98931.c
> @@ -1,6 +1,6 @@
> /* { dg-do assemble } */
> /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm"
> "-mcpu=*" } } */
> -/* { dg-options "-march=armv8.1-m.main -O3
> --param=max-completely-peeled-insns=1300 --save-temps" } */
> +/* { dg-options "-march=armv8.1-m.main -O3
> --param=max-completely-peeled-insns=1300 --save-temps -mthumb" } */
>
> extern long long a[][20][26][26][22];
>
>
[committed] rtl-ssa: Use right obstack for temporary allocation
I noticed while working on PR98863 that we were using the main obstack to allocate temporary uses. That was safe, but represents a kind of local memory leak. Tested on aarch64-linux-gnu and x86_64-linux-gnu, pushed as obvious. Richard gcc/ * rtl-ssa/accesses.cc (function_info::make_use_available): Use m_temp_obstack rather than m_obstack to allocate the temporary use. --- gcc/rtl-ssa/accesses.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc index 992a54c29a2..5023d55852f 100644 --- a/gcc/rtl-ssa/accesses.cc +++ b/gcc/rtl-ssa/accesses.cc @@ -1290,7 +1290,7 @@ function_info::make_use_available (use_info *use, bb_info *bb) // Create a temporary placeholder phi. This will become // permanent if the change is later committed. phi = allocate_temp (phi_insn, resource, 0); - auto *input = allocate (phi, resource, ultimate_def); + auto *input = allocate_temp (phi, resource, ultimate_def); input->m_is_temp = true; phi->m_is_temp = true; phi->make_degenerate (input);
[committed] libstdc++: Fix filesystem::rename on Windows [PR 98985]
The _wrename function won't overwrite an existing file, so use MoveFileEx instead. That allows renaming directories over files, which POSIX doesn't allow, so check for that case explicitly and report an error. Also document the deviation from the expected behaviour, and add a test for filesystem::rename which was previously missing. The Filesystem TS experimental::filesystem::rename doesn't have that extra code to handle directories correctly, so the relevant parts of the new test are not run on Windows. libstdc++-v3/ChangeLog: * doc/xml/manual/status_cxx2014.xml: Document implementation specific properties of std::experimental::filesystem::rename. * doc/xml/manual/status_cxx2017.xml: Document implementation specific properties of std::filesystem::rename. * doc/html/*: Regenerate. * src/c++17/fs_ops.cc (fs::rename): Implement correct behaviour for directories on Windows. * src/filesystem/ops-common.h (__gnu_posix::rename): Use MoveFileExW on Windows. * testsuite/27_io/filesystem/operations/rename.cc: New test. * testsuite/experimental/filesystem/operations/rename.cc: New test. Tested x86_64-linux and x86_64-w64-mingw32. Committed to trunk. commit 1dfd95f0a0ca1d9e6cbc00e6cbfd1fa20a98f312 Author: Jonathan Wakely Date: Fri Feb 12 15:13:02 2021 libstdc++: Fix filesystem::rename on Windows [PR 98985] The _wrename function won't overwrite an existing file, so use MoveFileEx instead. That allows renaming directories over files, which POSIX doesn't allow, so check for that case explicitly and report an error. Also document the deviation from the expected behaviour, and add a test for filesystem::rename which was previously missing. The Filesystem TS experimental::filesystem::rename doesn't have that extra code to handle directories correctly, so the relevant parts of the new test are not run on Windows. libstdc++-v3/ChangeLog: * doc/xml/manual/status_cxx2014.xml: Document implementation specific properties of std::experimental::filesystem::rename. * doc/xml/manual/status_cxx2017.xml: Document implementation specific properties of std::filesystem::rename. * doc/html/*: Regenerate. * src/c++17/fs_ops.cc (fs::rename): Implement correct behaviour for directories on Windows. * src/filesystem/ops-common.h (__gnu_posix::rename): Use MoveFileExW on Windows. * testsuite/27_io/filesystem/operations/rename.cc: New test. * testsuite/experimental/filesystem/operations/rename.cc: New test. diff --git a/libstdc++-v3/doc/xml/manual/status_cxx2014.xml b/libstdc++-v3/doc/xml/manual/status_cxx2014.xml index 2cf5f629efb..5dc287707d8 100644 --- a/libstdc++-v3/doc/xml/manual/status_cxx2014.xml +++ b/libstdc++-v3/doc/xml/manual/status_cxx2014.xml @@ -1717,9 +1717,33 @@ not in any particular release. - +Implementation Specific Behavior + + Filesystem TS + + 2.1 POSIX conformance [fs.conform.9945] + The behavior of the filesystem library implementation will depend on + the target operating system. Some features will not be supported + on some targets. Symbolic links and file permissions + are not supported on Windows. + + + 15.30 Rename [fs.op.rename] + On Windows, experimental::filesystem::rename + is implemented by calling MoveFileExW and so + does not meet the requirements of POSIX rename + when one or both of the paths resolves to an existing directory. + Specifically, it is possible to rename a directory so it replaces + a non-directory (POSIX requires an error in that case), + and it is not possible to rename a directory to replace another + directory (POSIX requires that to work if the directory being + replaced is empty). + + + + diff --git a/libstdc++-v3/doc/xml/manual/status_cxx2017.xml b/libstdc++-v3/doc/xml/manual/status_cxx2017.xml index b1c12bd3799..7f64c47bdfe 100644 --- a/libstdc++-v3/doc/xml/manual/status_cxx2017.xml +++ b/libstdc++-v3/doc/xml/manual/status_cxx2017.xml @@ -2992,7 +2992,8 @@ since C++14 and the implementation is complete. 30.10.2.1 [fs.conform.9945] The behavior of the filesystem library implementation will depend on the target operating system. Some features will not be supported - on some targets. + on some targets. Symbolic links and file permissions + are not supported on Windows. @@ -3025,6 +3026,18 @@ since C++14 and the implementation is complete. If !is_regular_file(p), an error is reported. + + 30.10.15.32 [fs.op.rename] + On Windows, filesystem::rename + is implemented by calling MoveFileExW and so + does not meet the requirements of POSIX rename + when one or both of the paths resolves to
Re: [committed] libstdc++: Re-enable workaround for _wstat64 bug [PR 88881]
On 10/02/21 16:58 +, Jonathan Wakely wrote: This wasn't fixed upstream for mingw-w64 so we still need the workaround. libstdc++-v3/ChangeLog: PR libstdc++/1 * src/c++17/fs_ops.cc (fs::status): Re-enable workaround. Oops, the same change is needed in symlink_status as well. Tested x86_64-w64-mingw32. Committed to trunk. commit b7210405ed8eb5fd723b2c99960dcc5f0aec89b4 Author: Jonathan Wakely Date: Wed Feb 10 16:51:34 2021 libstdc++: Re-enable workaround for _wstat64 bug, again [PR 1] I forgot that the workaround is present in both filesystem::status and filesystem::symlink_status. This restores it in the latter. libstdc++-v3/ChangeLog: PR libstdc++/1 * src/c++17/fs_ops.cc (fs::symlink_status): Re-enable workaround. diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc index 3e1671e611e..66207ae5e44 100644 --- a/libstdc++-v3/src/c++17/fs_ops.cc +++ b/libstdc++-v3/src/c++17/fs_ops.cc @@ -1537,7 +1537,6 @@ fs::symlink_status(const fs::path& p, std::error_code& ec) noexcept auto str = p.c_str(); #if _GLIBCXX_FILESYSTEM_IS_WINDOWS -#if ! defined __MINGW64_VERSION_MAJOR || __MINGW64_VERSION_MAJOR < 6 // stat() fails if there's a trailing slash (PR 1) path p2; if (p.has_relative_path() && !p.has_filename()) @@ -1554,7 +1553,6 @@ fs::symlink_status(const fs::path& p, std::error_code& ec) noexcept } str = p2.c_str(); } -#endif #endif stat_type st;
[PATCH 2/2] openacc: Strided array sections and components of derived-type arrays
This patch disallows selecting components of array sections in update
directives for OpenACC, as specified in OpenACC 3.0, "2.14.4. Update
Directive", "Restrictions":
"In Fortran, members of variables of derived type may appear, including
a subarray of a member. Members of subarrays of derived type may
not appear."
The diagnostic for attempting to use the same construct on other
directives has also been improved.
OK for mainline?
Thanks,
Julian
gcc/fortran/
* openmp.c (resolve_omp_clauses): Disallow selecting components of
arrays of derived type.
gcc/testsuite/
* gfortran.dg/goacc/array-with-dt-2.f90: Remove expected errors.
* gfortran.dg/goacc/array-with-dt-6.f90: New test.
* gfortran.dg/goacc/mapping-tests-2.f90: Update expected error.
libgomp/
* testsuite/libgomp.oacc-fortran/array-stride-dt-1.f90: Remove expected
errors.
---
gcc/fortran/openmp.c | 55 +++
.../gfortran.dg/goacc/array-with-dt-2.f90 | 5 +-
.../gfortran.dg/goacc/array-with-dt-6.f90 | 10
.../gfortran.dg/goacc/mapping-tests-2.f90 | 4 +-
.../array-stride-dt-1.f90 | 5 +-
5 files changed, 48 insertions(+), 31 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-6.f90
diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index aab17f0589f..9bcb1bf62ca 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -5174,17 +5174,29 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses
*omp_clauses,
"are allowed on ORDERED directive at %L",
>where);
}
- gfc_ref *array_ref = NULL;
+ gfc_ref *lastref = NULL, *lastslice = NULL;
bool resolved = false;
if (n->expr)
{
- array_ref = n->expr->ref;
+ lastref = n->expr->ref;
resolved = gfc_resolve_expr (n->expr);
/* Look through component refs to find last array
reference. */
if (resolved)
{
+ for (gfc_ref *ref = n->expr->ref; ref; ref = ref->next)
+ if (ref->type == REF_COMPONENT)
+ lastref = ref;
+ else if (ref->type == REF_ARRAY)
+ {
+ for (int i = 0; i < ref->u.ar.dimen; i++)
+ if (ref->u.ar.dimen_type[i] == DIMEN_RANGE)
+ lastslice = ref;
+
+ lastref = ref;
+ }
+
/* The "!$acc cache" directive allows rectangular
subarrays to be specified, with some restrictions
on the form of bounds (not implemented).
@@ -5192,45 +5204,42 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses
*omp_clauses,
array isn't contiguous. An expression such as
arr(-n:n,-n:n) could be contiguous even if it looks
like it may not be. */
- if (list != OMP_LIST_CACHE
+ if (code->op != EXEC_OACC_UPDATE
+ && list != OMP_LIST_CACHE
&& list != OMP_LIST_DEPEND
&& !gfc_is_simply_contiguous (n->expr, false, true)
- && gfc_is_not_contiguous (n->expr))
+ && gfc_is_not_contiguous (n->expr)
+ && !(lastslice
+&& (lastslice->next
+|| lastslice->type != REF_ARRAY)))
gfc_error ("Array is not contiguous at %L",
>where);
-
- while (array_ref
- && (array_ref->type == REF_COMPONENT
- || (array_ref->type == REF_ARRAY
- && array_ref->next
- && (array_ref->next->type
- == REF_COMPONENT
- array_ref = array_ref->next;
}
}
- if (array_ref
+ if (lastref
|| (n->expr
&& (!resolved || n->expr->expr_type != EXPR_VARIABLE)))
{
- if (array_ref
- && (array_ref->type == REF_SUBSTRING
- || (array_ref->next
- && array_ref->next->type == REF_SUBSTRING)))
+ if (lastref
+ && (lastref->type == REF_SUBSTRING
+ ||
[PATCH 1/2] openacc: Fix lowering for derived-type mappings through array elements
This patch fixes lowering of derived-type mappings which select elements
of arrays of derived types, and similar. These would previously lead
to ICEs.
With this change, OpenACC directives can pass through constructs that
are no longer recognized by the gimplifier, hence alterations are needed
there also.
OK for mainline?
Thanks,
Julian
gcc/fortran/
* trans-openmp.c (gfc_trans_omp_clauses): Handle element selection
for arrays of derived types.
gcc/
* gimplify.c (gimplify_scan_omp_clauses): Handle ATTACH_DETACH
for non-decls.
gcc/testsuite/
* gfortran.dg/goacc/array-with-dt-1.f90: New test.
* gfortran.dg/goacc/array-with-dt-3.f90: Likewise.
* gfortran.dg/goacc/array-with-dt-4.f90: Likewise.
* gfortran.dg/goacc/array-with-dt-5.f90: Likewise.
* gfortran.dg/goacc/derived-chartypes-1.f90: Re-enable test.
* gfortran.dg/goacc/derived-chartypes-2.f90: Likewise.
* gfortran.dg/goacc/derived-classtypes-1.f95: Uncomment
previously-broken directives.
libgomp/
* testsuite/libgomp.oacc-fortran/derivedtypes-arrays-1.f90: New test.
* testsuite/libgomp.oacc-fortran/update-dt-array.f90: Likewise.
---
gcc/fortran/trans-openmp.c| 192 ++
gcc/gimplify.c| 12 ++
.../gfortran.dg/goacc/array-with-dt-1.f90 | 11 +
.../gfortran.dg/goacc/array-with-dt-3.f90 | 14 ++
.../gfortran.dg/goacc/array-with-dt-4.f90 | 18 ++
.../gfortran.dg/goacc/array-with-dt-5.f90 | 12 ++
.../gfortran.dg/goacc/derived-chartypes-1.f90 | 3 -
.../gfortran.dg/goacc/derived-chartypes-2.f90 | 3 -
.../goacc/derived-classtypes-1.f95| 8 +-
.../derivedtypes-arrays-1.f90 | 109 ++
.../libgomp.oacc-fortran/update-dt-array.f90 | 53 +
11 files changed, 344 insertions(+), 91 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90
create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-3.f90
create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-4.f90
create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-5.f90
create mode 100644
libgomp/testsuite/libgomp.oacc-fortran/derivedtypes-arrays-1.f90
create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/update-dt-array.f90
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
index 249b3de2cfd..67e370f8b57 100644
--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -2675,6 +2675,32 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
tree decl = gfc_trans_omp_variable (n->sym, false);
if (DECL_P (decl))
TREE_ADDRESSABLE (decl) = 1;
+
+ gfc_ref *lastref = NULL;
+
+ if (n->expr)
+ for (gfc_ref *ref = n->expr->ref; ref; ref = ref->next)
+ if (ref->type == REF_COMPONENT || ref->type == REF_ARRAY)
+ lastref = ref;
+
+ bool allocatable = false, pointer = false;
+
+ if (lastref && lastref->type == REF_COMPONENT)
+ {
+ gfc_component *c = lastref->u.c.component;
+
+ if (c->ts.type == BT_CLASS)
+ {
+ pointer = CLASS_DATA (c)->attr.class_pointer;
+ allocatable = CLASS_DATA (c)->attr.allocatable;
+ }
+ else
+ {
+ pointer = c->attr.pointer;
+ allocatable = c->attr.allocatable;
+ }
+ }
+
if (n->expr == NULL
|| (n->expr->ref->type == REF_ARRAY
&& n->expr->ref->u.ar.type == AR_FULL))
@@ -2911,74 +2937,79 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
}
else if (n->expr
&& n->expr->expr_type == EXPR_VARIABLE
- && n->expr->ref->type == REF_COMPONENT)
+ && n->expr->ref->type == REF_ARRAY
+ && !n->expr->ref->next)
{
- gfc_ref *lastcomp;
-
- for (gfc_ref *ref = n->expr->ref; ref; ref = ref->next)
- if (ref->type == REF_COMPONENT)
- lastcomp = ref;
-
- symbol_attribute sym_attr;
-
- if (lastcomp->u.c.component->ts.type == BT_CLASS)
- sym_attr = CLASS_DATA (lastcomp->u.c.component)->attr;
- else
- sym_attr = lastcomp->u.c.component->attr;
-
+ /* An array element or array section which is not part of a
+derived type, etc. */
+ bool element = n->expr->ref->u.ar.type == AR_ELEMENT;
+ gfc_trans_omp_array_section (block, n, decl, element,
+
[PATCH 0/2] openacc: Mixing array elements and derived types (mk2)
This series contains an updated version of the "3/4" patch from this series: https://gcc.gnu.org/pipermail/gcc-patches/2021-February/564711.html together with bits that undo the reversions in: https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565093.html and a new approach to handling components of array sections of derived types, since those are actually already explicitly disallowed by the spec (OpenACC 3.0). (Actually the 1/2 patch is the same as the previously-posted version, apart from testsuite changes.) Re-tested with offloading to AMD GCN. Further commentary on individual patches. OK? Thanks, Julian Julian Brown (2): openacc: Fix lowering for derived-type mappings through array elements openacc: Strided array sections and components of derived-type arrays gcc/fortran/openmp.c | 55 ++--- gcc/fortran/trans-openmp.c| 192 ++ gcc/gimplify.c| 12 ++ .../gfortran.dg/goacc/array-with-dt-1.f90 | 11 + .../gfortran.dg/goacc/array-with-dt-2.f90 | 5 +- .../gfortran.dg/goacc/array-with-dt-3.f90 | 14 ++ .../gfortran.dg/goacc/array-with-dt-4.f90 | 18 ++ .../gfortran.dg/goacc/array-with-dt-5.f90 | 12 ++ .../gfortran.dg/goacc/array-with-dt-6.f90 | 10 + .../gfortran.dg/goacc/derived-chartypes-1.f90 | 3 - .../gfortran.dg/goacc/derived-chartypes-2.f90 | 3 - .../goacc/derived-classtypes-1.f95| 8 +- .../gfortran.dg/goacc/mapping-tests-2.f90 | 4 +- .../array-stride-dt-1.f90 | 5 +- .../derivedtypes-arrays-1.f90 | 109 ++ .../libgomp.oacc-fortran/update-dt-array.f90 | 53 + 16 files changed, 392 insertions(+), 122 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90 create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-3.f90 create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-4.f90 create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-5.f90 create mode 100644 gcc/testsuite/gfortran.dg/goacc/array-with-dt-6.f90 create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/derivedtypes-arrays-1.f90 create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/update-dt-array.f90 -- 2.29.2
Re: [committed] libstdc++: Include scope ID in net::internet::address_v6::to_string()
On 12/02/21 15:12 +, Jonathan Wakely wrote:
@@ -364,9 +378,7 @@ namespace ip
static constexpr address_v6
loopback() noexcept
{
- address_v6 __addr;
- __addr._M_bytes[15] = 1;
- return __addr;
+ return {bytes_type{0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1}};
}
Oops, this was supposed to be a separate commit. The previous version
was only constexpr in C++17 and later, which is why I changed it.
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
Nope. I can't reach Robert, so CC MIPS maintainer. On 2021-02-12 22:57 +0800，Xi Ruoyao wrote: > Well, it just dislike my mail server :(. Switch to the mail server of my > university. > > On 2021-02-12 22:54 +0800, Xi Ruoyao wrote: > > Resend the mail. I had to fill in a form to send mail to Robert. > > > > On 2021-02-12 22:17 +0800, Xi Ruoyao wrote: > > > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote: > > > > Hi Jeff and Jakub, > > > > > > > > On 2021-01-04 14:19 -0700, Jeff Law wrote: > > > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote: > > > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches > > > > > > wrote: > > > > > > > > Sorry, I forgot to include the ChangeLog: > > > > > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > > > > > 2020-12-31 Xi Ruoyao > > > > > > > > > > > > > > > > PR target/98491 > > > > > > > > * config/mips/mips.c (mips_symbol_insns): Do not use > > > > > > > > MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE. > > > > > > > So I absolutely agree the current code is wrong as it does an out > > > > > > > of > > > > > > > bounds array access. > > > > > > > > > > > > > > > > > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to > > > > > > > evaluate > > > > > > > to zero if MODE is MAX_MACHINE_MODE? That would protect all the > > > > > > > uses > > > > > > > of > > > > > > > MSA_SUPPORTED_MODE_P. Something like this perhaps? > > > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware > > > > > > of > > > > > > any target that would protect all macros that deal with modes that > > > > > > way. > > > > > > > > > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic > > > > > > value > > > > > > for that function and instead use say VOIDmode that shouldn't > > > > > > normally > > > > > > appear either? > > > > > I think we have to allow VOIDmode because constants don't necessarily > > > > > have modes. And I certainly agree that using MAX_MACHINE_MODE like > > > > > this is ugly and error prone (as we can see from the BZ). > > > > > > > > > > I also couldn't convince myself that the code and comments were > > > > > actually > > > > > consistent, particularly for MSA targets which the comment claims can > > > > > never handle constants for ld/st (and thus should be returning 0 for > > > > > MAX_MACHINE_MODE). Though maybe mips_symbol_insns_1 ultimately > > > > > handles > > > > > that correctly. > > > > > > > > > > > > > > > > > > > > > > But I don't really see anything wrong on the mips_symbol_insns above > > > > > > change either. > > > > > Me neither. I'm just questioning if bullet-proofing in the > > > > > MSA_SUPPORTED_MODE_P would be a better option. While I've worked in > > > > > the > > > > > MIPS port in the past, I don't really have any significannt experience > > > > > with the MSA support. > > > > > > > > I can't understand the comment either. To me it looks like it's > > > > possible > > > > to > > > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;" > > > > > > > > CC Robert to get some help. > > > > > > Happy new lunar year folks. > > > > > > I found a newer email address of Robert. Hope it is still being used. > > > > > > Could someone update MAINTAINERS file by the way? > > > > -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
[committed] libstdc++: Make "nonexistent" paths less predictable in filesystem tests
The helper function for creating new paths doesn't work well on Windows,
because the PID of a process started by Wine is very consistent and so
the same path gets created each time.
libstdc++-v3/ChangeLog:
* testsuite/util/testsuite_fs.h (nonexistent_path): Add
random number to the path.
Tested x86_64-linux and x86_64-w64-mingw32. Committed to trunk.
commit 4179ec107943bea360b8aa75e29e2c5ad9f13e9e
Author: Jonathan Wakely
Date: Fri Feb 12 15:13:02 2021
libstdc++: Make "nonexistent" paths less predictable in filesystem tests
The helper function for creating new paths doesn't work well on Windows,
because the PID of a process started by Wine is very consistent and so
the same path gets created each time.
libstdc++-v3/ChangeLog:
* testsuite/util/testsuite_fs.h (nonexistent_path): Add
random number to the path.
diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h
b/libstdc++-v3/testsuite/util/testsuite_fs.h
index 4cda0eefeaf..e4d04dd7799 100644
--- a/libstdc++-v3/testsuite/util/testsuite_fs.h
+++ b/libstdc++-v3/testsuite/util/testsuite_fs.h
@@ -34,8 +34,13 @@ namespace test_fs = std::experimental::filesystem;
#include
#include
#include
-#include
-#include
+
+#if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L
+#include // mkstemp
+#include // unlink, close
+#else
+#include// std::random_device
+#endif
namespace __gnu_test
{
@@ -121,13 +126,13 @@ namespace __gnu_test
if (file.length() > 64)
file.resize(64);
char buf[128];
-static int counter;
+static unsigned counter = std::random_device{}();
#if _GLIBCXX_USE_C99_STDIO
std::snprintf(buf, 128,
#else
std::sprintf(buf,
#endif
- "filesystem-test.%d.%lu-%s", counter++, (unsigned long) ::getpid(),
+ "filesystem-test.%u.%lu-%s", counter++, (unsigned long) ::getpid(),
file.c_str());
p = buf;
#endif
[committed] libstdc++: Include scope ID in net::internet::address_v6::to_string()
libstdc++-v3/ChangeLog:
* include/experimental/internet (address_v6::to_string): Include
scope ID in string.
* testsuite/experimental/net/internet/address/v6/members.cc:
Test to_string() results.
Tested powerpc64le-linux. Committed to trunk.
commit d1a821b93c45bfe7606b5dee8d160c7172b37e3e
Author: Jonathan Wakely
Date: Fri Feb 12 15:08:29 2021
libstdc++: Include scope ID in net::internet::address_v6::to_string()
libstdc++-v3/ChangeLog:
* include/experimental/internet (address_v6::to_string): Include
scope ID in string.
* testsuite/experimental/net/internet/address/v6/members.cc:
Test to_string() results.
diff --git a/libstdc++-v3/include/experimental/internet
b/libstdc++-v3/include/experimental/internet
index 57831676a29..288c61ba25a 100644
--- a/libstdc++-v3/include/experimental/internet
+++ b/libstdc++-v3/include/experimental/internet
@@ -344,9 +344,23 @@ namespace ip
to_string(const _Allocator& __a = _Allocator()) const
{
__string_with<_Allocator> __str(__a);
- __str.resize(INET6_ADDRSTRLEN);
- if (inet_ntop(AF_INET6, &_M_bytes, &__str.front(), __str.size()))
- __str.erase(__str.find('\0'));
+ __str.resize(INET6_ADDRSTRLEN + (_M_scope_id ? 11 : 0));
+ char* const __p = &__str.front();
+ if (inet_ntop(AF_INET6, &_M_bytes, __p, __str.size()))
+ {
+ auto __end = __str.find('\0');
+ if (unsigned long __scope = _M_scope_id)
+ {
+ __end +=
+#if _GLIBCXX_USE_C99_STDIO
+ __builtin_snprintf(__p + __end, __str.size() - __end,
+"%%%lu", __scope);
+#else
+ __builtin_sprintf(__p + __end, "%%%lu", __scope);
+#endif
+ }
+ __str.erase(__end);
+ }
else
__str.resize(0);
return __str;
@@ -364,9 +378,7 @@ namespace ip
static constexpr address_v6
loopback() noexcept
{
- address_v6 __addr;
- __addr._M_bytes[15] = 1;
- return __addr;
+ return {bytes_type{0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1}};
}
private:
diff --git
a/libstdc++-v3/testsuite/experimental/net/internet/address/v6/members.cc
b/libstdc++-v3/testsuite/experimental/net/internet/address/v6/members.cc
index 506345dc990..b77d6a29e3d 100644
--- a/libstdc++-v3/testsuite/experimental/net/internet/address/v6/members.cc
+++ b/libstdc++-v3/testsuite/experimental/net/internet/address/v6/members.cc
@@ -86,16 +86,30 @@ static_assert(test02(), "");
void
test03()
{
+ // Assume these addresses use the preferred forms:
VERIFY( address_v6::any().to_string() == "::" );
VERIFY( address_v6::loopback().to_string() == "::1" );
+
+ // Choose values with no leading zeros, so output is portable:
+ address_v6 a{address_v6::bytes_type{21,22,23,24,25,26,27,28,29}, 42};
+ const std::string s = a.to_string();
+ if (s.find("::") != s.npos)
+VERIFY( s == "1516:1718:191a:1b1c:1d00::%42" );
+ else
+ {
+// Contiguous zeros were not shortened to "::"
+VERIFY( s.substr(0, 25) == "1516:1718:191a:1b1c:1d00:" );
+VERIFY( s.substr(s.size() - 3) == "%42" );
+ }
}
void
test04()
{
+ address_v6 a{address_v6::bytes_type{1,2,3,4,5,6,7,8,9}, 42};
std::ostringstream ss;
- ss << address_v6::any() << ' ' << address_v6::loopback();
- VERIFY( ss.str() == ":: ::1" );
+ ss << address_v6::any() << ' ' << address_v6::loopback() << ' ' << a;
+ VERIFY( ss.str() == (":: ::1 " + a.to_string()) );
}
int
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
Well, it just dislike my mail server :(. Switch to the mail server of my university. On 2021-02-12 22:54 +0800, Xi Ruoyao wrote: > Resend the mail. I had to fill in a form to send mail to Robert. > > On 2021-02-12 22:17 +0800, Xi Ruoyao wrote: > > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote: > > > Hi Jeff and Jakub, > > > > > > On 2021-01-04 14:19 -0700, Jeff Law wrote: > > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote: > > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches > > > > > wrote: > > > > > > > Sorry, I forgot to include the ChangeLog: > > > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > > > 2020-12-31 Xi Ruoyao > > > > > > > > > > > > > > PR target/98491 > > > > > > > * config/mips/mips.c (mips_symbol_insns): Do not use > > > > > > > MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE. > > > > > > So I absolutely agree the current code is wrong as it does an out of > > > > > > bounds array access. > > > > > > > > > > > > > > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to > > > > > > evaluate > > > > > > to zero if MODE is MAX_MACHINE_MODE? That would protect all the > > > > > > uses > > > > > > of > > > > > > MSA_SUPPORTED_MODE_P. Something like this perhaps? > > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of > > > > > any target that would protect all macros that deal with modes that > > > > > way. > > > > > > > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic > > > > > value > > > > > for that function and instead use say VOIDmode that shouldn't normally > > > > > appear either? > > > > I think we have to allow VOIDmode because constants don't necessarily > > > > have modes. And I certainly agree that using MAX_MACHINE_MODE like > > > > this is ugly and error prone (as we can see from the BZ). > > > > > > > > I also couldn't convince myself that the code and comments were actually > > > > consistent, particularly for MSA targets which the comment claims can > > > > never handle constants for ld/st (and thus should be returning 0 for > > > > MAX_MACHINE_MODE). Though maybe mips_symbol_insns_1 ultimately handles > > > > that correctly. > > > > > > > > > > > > > > > > > > But I don't really see anything wrong on the mips_symbol_insns above > > > > > change either. > > > > Me neither. I'm just questioning if bullet-proofing in the > > > > MSA_SUPPORTED_MODE_P would be a better option. While I've worked in the > > > > MIPS port in the past, I don't really have any significannt experience > > > > with the MSA support. > > > > > > I can't understand the comment either. To me it looks like it's possible > > > to > > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;" > > > > > > CC Robert to get some help. > > > > Happy new lunar year folks. > > > > I found a newer email address of Robert. Hope it is still being used. > > > > Could someone update MAINTAINERS file by the way? >
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
Resend the mail. I had to fill in a form to send mail to Robert. On 2021-02-12 22:17 +0800, Xi Ruoyao wrote: > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote: > > Hi Jeff and Jakub, > > > > On 2021-01-04 14:19 -0700, Jeff Law wrote: > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote: > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches > > > > wrote: > > > > > > Sorry, I forgot to include the ChangeLog: > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > 2020-12-31 Xi Ruoyao > > > > > > > > > > > > PR target/98491 > > > > > > * config/mips/mips.c (mips_symbol_insns): Do not use > > > > > > MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE. > > > > > So I absolutely agree the current code is wrong as it does an out of > > > > > bounds array access. > > > > > > > > > > > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to > > > > > evaluate > > > > > to zero if MODE is MAX_MACHINE_MODE? That would protect all the uses > > > > > of > > > > > MSA_SUPPORTED_MODE_P. Something like this perhaps? > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of > > > > any target that would protect all macros that deal with modes that way. > > > > > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value > > > > for that function and instead use say VOIDmode that shouldn't normally > > > > appear either? > > > I think we have to allow VOIDmode because constants don't necessarily > > > have modes. And I certainly agree that using MAX_MACHINE_MODE like > > > this is ugly and error prone (as we can see from the BZ). > > > > > > I also couldn't convince myself that the code and comments were actually > > > consistent, particularly for MSA targets which the comment claims can > > > never handle constants for ld/st (and thus should be returning 0 for > > > MAX_MACHINE_MODE). Though maybe mips_symbol_insns_1 ultimately handles > > > that correctly. > > > > > > > > > > > > > > But I don't really see anything wrong on the mips_symbol_insns above > > > > change either. > > > Me neither. I'm just questioning if bullet-proofing in the > > > MSA_SUPPORTED_MODE_P would be a better option. While I've worked in the > > > MIPS port in the past, I don't really have any significannt experience > > > with the MSA support. > > > > I can't understand the comment either. To me it looks like it's possible to > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;" > > > > CC Robert to get some help. > > Happy new lunar year folks. > > I found a newer email address of Robert. Hope it is still being used. > > Could someone update MAINTAINERS file by the way? -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
[committed] libstdc++: Add unused attributes to shared_ptr functions
This avoids some warnings when building with -fno-rtti because the
function parameters are only used when RTTI is enabled.
libstdc++-v3/ChangeLog:
* include/bits/shared_ptr_base.h (__shared_ptr::_M_get_deleter):
Add unused attribute to parameter.
* src/c++11/shared_ptr.cc (_Sp_make_shared_tag::_S_eq):
Likewise.
Tested powerpc64le-linux. Committed to trunk.
commit 87eaa3c525eb65775e6d77403b83a273a2397099
Author: Jonathan Wakely
Date: Fri Feb 12 10:36:18 2021
libstdc++: Add unused attributes to shared_ptr functions
This avoids some warnings when building with -fno-rtti because the
function parameters are only used when RTTI is enabled.
libstdc++-v3/ChangeLog:
* include/bits/shared_ptr_base.h (__shared_ptr::_M_get_deleter):
Add unused attribute to parameter.
* src/c++11/shared_ptr.cc (_Sp_make_shared_tag::_S_eq):
Likewise.
diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h
b/libstdc++-v3/include/bits/shared_ptr_base.h
index 15707f8e74a..b24900b2008 100644
--- a/libstdc++-v3/include/bits/shared_ptr_base.h
+++ b/libstdc++-v3/include/bits/shared_ptr_base.h
@@ -450,7 +450,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
virtual void*
- _M_get_deleter(const std::type_info& __ti) noexcept
+ _M_get_deleter(const type_info& __ti [[__gnu__::__unused__]]) noexcept
{
#if __cpp_rtti
// _GLIBCXX_RESOLVE_LIB_DEFECTS
diff --git a/libstdc++-v3/src/c++11/shared_ptr.cc
b/libstdc++-v3/src/c++11/shared_ptr.cc
index 13e2d520199..4678fbeffe2 100644
--- a/libstdc++-v3/src/c++11/shared_ptr.cc
+++ b/libstdc++-v3/src/c++11/shared_ptr.cc
@@ -97,7 +97,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#endif
bool
- _Sp_make_shared_tag::_S_eq(const type_info& ti) noexcept
+ _Sp_make_shared_tag::_S_eq(const type_info& ti [[gnu::unused]]) noexcept
{
#if __cpp_rtti
return ti == typeid(_Sp_make_shared_tag);
[committed] libstdc++: Fix errors in
libstdc++-v3/ChangeLog:
* include/experimental/internet (address_v6::any): Avoid using
memcpy in constexpr function.
(address_v6::loopback): Likewise.
(make_address_v6): Fix missing return statements on error paths.
* include/experimental/io_context: Avoid -Wdangling-else
warning.
* testsuite/experimental/net/internet/address/v4/members.cc:
Remove unused variables.
* testsuite/experimental/net/internet/address/v6/members.cc:
New test.
Tested powerpc64le-linux. Committed to trunk.
commit 970ba719250ec06767e0617658bb92a64fde0f3f
Author: Jonathan Wakely
Date: Fri Feb 12 13:01:20 2021
libstdc++: Fix errors in
libstdc++-v3/ChangeLog:
* include/experimental/internet (address_v6::any): Avoid using
memcpy in constexpr function.
(address_v6::loopback): Likewise.
(make_address_v6): Fix missing return statements on error paths.
* include/experimental/io_context: Avoid -Wdangling-else
warning.
* testsuite/experimental/net/internet/address/v4/members.cc:
Remove unused variables.
* testsuite/experimental/net/internet/address/v6/members.cc:
New test.
diff --git a/libstdc++-v3/include/experimental/internet
b/libstdc++-v3/include/experimental/internet
index 2fc253395f5..57831676a29 100644
--- a/libstdc++-v3/include/experimental/internet
+++ b/libstdc++-v3/include/experimental/internet
@@ -354,19 +354,18 @@ namespace ip
#endif
// static members:
+
static constexpr address_v6
any() noexcept
{
- address_v6 __addr;
- __builtin_memcpy(&__addr._M_bytes, in6addr_any.s6_addr, 16);
- return __addr;
+ return {};
}
static constexpr address_v6
loopback() noexcept
{
address_v6 __addr;
- __builtin_memcpy(&__addr._M_bytes, in6addr_loopback.s6_addr, 16);
+ __addr._M_bytes[15] = 1;
return __addr;
}
@@ -755,7 +754,10 @@ namespace ip
__str++;
}
if (__out == std::end(__buf))
- __ec = std::make_error_code(std::errc::invalid_argument);
+ {
+ __ec = std::make_error_code(std::errc::invalid_argument);
+ return {};
+ }
else
{
*__out = '\0';
@@ -790,7 +792,10 @@ namespace ip
__n++;
}
if (__out == std::end(__buf))
- __ec = std::make_error_code(std::errc::invalid_argument);
+ {
+ __ec = std::make_error_code(std::errc::invalid_argument);
+ return {};
+ }
else
{
*__out = '\0';
@@ -835,7 +840,10 @@ namespace ip
__n++;
}
if (__out == std::end(__buf))
- __ec = std::make_error_code(std::errc::invalid_argument);
+ {
+ __ec = std::make_error_code(std::errc::invalid_argument);
+ return {};
+ }
else
{
*__out = '\0';
diff --git a/libstdc++-v3/include/experimental/io_context
b/libstdc++-v3/include/experimental/io_context
index 12c269b4af2..c82f30cd119 100644
--- a/libstdc++-v3/include/experimental/io_context
+++ b/libstdc++-v3/include/experimental/io_context
@@ -680,10 +680,12 @@ inline namespace v1
continue;
if (__res == __reactor::_S_timeout)
- if (__timerq == nullptr)
- return false;
- else
- continue; // timed out, so restart loop and process the timer
+ {
+ if (__timerq == nullptr)
+ return false;
+ else
+ continue; // timed out, so restart loop and process the timer
+ }
__timerq = nullptr;
diff --git
a/libstdc++-v3/testsuite/experimental/net/internet/address/v4/members.cc
b/libstdc++-v3/testsuite/experimental/net/internet/address/v4/members.cc
index f0eff14f202..f644c0847ab 100644
--- a/libstdc++-v3/testsuite/experimental/net/internet/address/v4/members.cc
+++ b/libstdc++-v3/testsuite/experimental/net/internet/address/v4/members.cc
@@ -19,15 +19,14 @@
// { dg-add-options net_ts }
#include
+#include
#include
using std::experimental::net::ip::address_v4;
-void
+constexpr bool
test01()
{
- bool test __attribute__((unused)) = false;
-
address_v4 a;
VERIFY( a.is_unspecified() );
@@ -39,13 +38,15 @@ test01()
a = address_v4::broadcast();
VERIFY( !a.is_unspecified() );
+
+ return true;
}
-void
+static_assert(test01(), "");
+
+constexpr bool
test02()
{
- bool test __attribute__((unused)) = false;
-
auto a = address_v4::loopback();
VERIFY( a.is_loopback() );
@@ -63,13 +64,15 @@ test02()
a = address_v4::broadcast();
VERIFY( !a.is_loopback() );
+
+ return true;
}
-void
+static_assert(test02(), "");
+
+constexpr bool
test03()
{
- bool test __attribute__((unused)) = false;
-
auto a = address_v4{0xE001};
VERIFY( a.is_multicast() );
@@ -84,13 +87,15 @@ test03()
a = address_v4{0xDFFF};
VERIFY( !a.is_multicast() );
+
+
[committed] libstdc++: XFAIL tests that depends on RTTI
The std::emit_on_flush manipulator depends on dynamic_cast, so fails
without RTTI.
The std::async code can't catch a forced_unwind exception when RTTI is
disabled, so it can't rethrow it either, and the test aborts.
libstdc++-v3/ChangeLog:
* testsuite/27_io/basic_ostream/emit/1.cc: Expect test to fail
if -fno-rtti is used.
* testsuite/30_threads/async/forced_unwind.cc: Expect test
to abort if -fno-rtti is used.
Tested powerpc64le-linux. Committed to trunk.
commit c4ece1d96a105f51d7999b7afe9340d582731f5d
Author: Jonathan Wakely
Date: Fri Feb 12 11:30:38 2021
libstdc++: XFAIL tests that depends on RTTI
The std::emit_on_flush manipulator depends on dynamic_cast, so fails
without RTTI.
The std::async code can't catch a forced_unwind exception when RTTI is
disabled, so it can't rethrow it either, and the test aborts.
libstdc++-v3/ChangeLog:
* testsuite/27_io/basic_ostream/emit/1.cc: Expect test to fail
if -fno-rtti is used.
* testsuite/30_threads/async/forced_unwind.cc: Expect test
to abort if -fno-rtti is used.
diff --git a/libstdc++-v3/testsuite/27_io/basic_ostream/emit/1.cc
b/libstdc++-v3/testsuite/27_io/basic_ostream/emit/1.cc
index d692c53b392..ac813942a47 100644
--- a/libstdc++-v3/testsuite/27_io/basic_ostream/emit/1.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_ostream/emit/1.cc
@@ -19,6 +19,7 @@
// { dg-additional-options "-pthread" { target pthread } }
// { dg-do run { target c++2a } }
// { dg-require-effective-target cxx11-abi }
+// { dg-xfail-run-if "cannot catch forced_unwind" { *-*-* } { "-fno-rtti" } }
#include
#include
diff --git a/libstdc++-v3/testsuite/30_threads/async/forced_unwind.cc
b/libstdc++-v3/testsuite/30_threads/async/forced_unwind.cc
index ad7c8ff0232..2cc477850ce 100644
--- a/libstdc++-v3/testsuite/30_threads/async/forced_unwind.cc
+++ b/libstdc++-v3/testsuite/30_threads/async/forced_unwind.cc
@@ -3,6 +3,7 @@
// { dg-require-effective-target c++11 }
// { dg-require-effective-target pthread }
// { dg-require-gthreads "" }
+// { dg-xfail-run-if "cannot catch forced_unwind" { *-*-* } { "-fno-rtti" } }
// Copyright (C) 2014-2021 Free Software Foundation, Inc.
//
[committed] libstdc++: Fix errors when syncbuf is used without RTTI
libstdc++-v3/ChangeLog:
* include/std/ostream (__syncbuf_base::_S_get): Mark parameter
as unused and only use dynamic_cast when RTTI is enabled.
Tested powerpc64le-linux. Committed to trunk.
commit 14b554c462d5b6450fa24afb7ba55435ebd4b46f
Author: Jonathan Wakely
Date: Fri Feb 12 11:36:27 2021
libstdc++: Fix errors when syncbuf is used without RTTI
libstdc++-v3/ChangeLog:
* include/std/ostream (__syncbuf_base::_S_get): Mark parameter
as unused and only use dynamic_cast when RTTI is enabled.
diff --git a/libstdc++-v3/include/std/ostream b/libstdc++-v3/include/std/ostream
index 85ed47ecbce..c7c4e78e8a7 100644
--- a/libstdc++-v3/include/std/ostream
+++ b/libstdc++-v3/include/std/ostream
@@ -783,10 +783,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{
public:
static bool*
- _S_get(basic_streambuf<_CharT, _Traits>* __buf) noexcept
+ _S_get(basic_streambuf<_CharT, _Traits>* __buf [[maybe_unused]]) noexcept
{
+#if __cpp_rtti
if (auto __p = dynamic_cast<__syncbuf_base*>(__buf))
return &__p->_M_emit_on_sync;
+#endif
return nullptr;
}
[committed] libstdc++: Make test memory_resource work without exceptions and RTTI
libstdc++-v3/ChangeLog:
* testsuite/util/testsuite_allocator.h (memory_resource):
Remove requirement for RTTI and exceptions to be enabled.
Tested powerpc64le-linux. Committed to trunk.
commit 0bd242ec5aeffd1fb2a3ee16a2c69afae2aff2ce
Author: Jonathan Wakely
Date: Fri Feb 12 11:23:28 2021
libstdc++: Make test memory_resource work without exceptions and RTTI
libstdc++-v3/ChangeLog:
* testsuite/util/testsuite_allocator.h (memory_resource):
Remove requirement for RTTI and exceptions to be enabled.
diff --git a/libstdc++-v3/testsuite/util/testsuite_allocator.h
b/libstdc++-v3/testsuite/util/testsuite_allocator.h
index 9f80a14beb0..1f7912ea6eb 100644
--- a/libstdc++-v3/testsuite/util/testsuite_allocator.h
+++ b/libstdc++-v3/testsuite/util/testsuite_allocator.h
@@ -763,7 +763,7 @@ namespace __gnu_test
#endif // C++11
#if __cplusplus >= 201703L
-#if __cpp_aligned_new && __cpp_rtti && __cpp_exceptions
+#if __cpp_aligned_new
// A concrete memory_resource, with error checking.
class memory_resource : public std::pmr::memory_resource
{
@@ -842,9 +842,9 @@ namespace __gnu_test
if (p == a->p)
{
if (bytes != a->bytes)
- throw bad_size();
+ _S_throw();
if (alignment != a->alignment)
- throw bad_alignment();
+ _S_throw();
#if __cpp_sized_deallocation
::operator delete(p, bytes, std::align_val_t(alignment));
#else
@@ -857,19 +857,35 @@ namespace __gnu_test
}
aptr = >next;
}
- throw bad_address();
+ _S_throw();
}
bool
do_is_equal(const std::pmr::memory_resource& r) const noexcept override
{
+#if __cpp_rtti
// Equality is determined by sharing the same allocation_lists object.
if (auto p = dynamic_cast())
return p->lists == lists;
+#else
+ if (this == ) // Is this the best we can do without RTTI?
+ return true;
+#endif
return false;
}
private:
+template
+ static void
+ _S_throw()
+ {
+#if __cpp_exceptions
+ throw E();
+#else
+ __builtin_abort();
+#endif
+ }
+
struct allocation
{
void* p;
@@ -905,7 +921,7 @@ namespace __gnu_test
allocation_lists* lists;
};
-#endif // aligned-new && rtti
+#endif // aligned-new
// Set the default resource, and restore the previous one on destruction.
struct default_resource_mgr
[committed] libstdc++: Only use dynamic_cast in tests when RTTI is enabled
libstdc++-v3/ChangeLog:
* testsuite/27_io/basic_istringstream/rdbuf/char/2832.cc: Use
static_cast when RTTI is disabled.
* testsuite/27_io/basic_istringstream/rdbuf/wchar_t/2832.cc:
Likewise.
* testsuite/27_io/basic_ostringstream/rdbuf/char/2832.cc:
Likewise.
* testsuite/27_io/basic_ostringstream/rdbuf/wchar_t/2832.cc:
Likewise.
* testsuite/27_io/basic_stringstream/str/char/2.cc:
Likewise.
* testsuite/27_io/basic_stringstream/str/wchar_t/2.cc:
Likewise.
Tested powerpc64le-linux. Committed to trunk.
commit e9c310521182c9359cf23fa9b0c5057b35e8b53f
Author: Jonathan Wakely
Date: Fri Feb 12 11:09:00 2021
libstdc++: Only use dynamic_cast in tests when RTTI is enabled
libstdc++-v3/ChangeLog:
* testsuite/27_io/basic_istringstream/rdbuf/char/2832.cc: Use
static_cast when RTTI is disabled.
* testsuite/27_io/basic_istringstream/rdbuf/wchar_t/2832.cc:
Likewise.
* testsuite/27_io/basic_ostringstream/rdbuf/char/2832.cc:
Likewise.
* testsuite/27_io/basic_ostringstream/rdbuf/wchar_t/2832.cc:
Likewise.
* testsuite/27_io/basic_stringstream/str/char/2.cc:
Likewise.
* testsuite/27_io/basic_stringstream/str/wchar_t/2.cc:
Likewise.
diff --git
a/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/char/2832.cc
b/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/char/2832.cc
index 00f732fee57..87677c50a13 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/char/2832.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/char/2832.cc
@@ -22,8 +22,8 @@
#include
#include
-void
-redirect_buffer(std::ios& stream, std::streambuf* new_buf)
+void
+redirect_buffer(std::ios& stream, std::streambuf* new_buf)
{ stream.rdbuf(new_buf); }
std::streambuf*
@@ -50,7 +50,7 @@ void test02()
redirect_buffer(sstrm1, );
std::stringbuf* const buf2 = sstrm1.rdbuf();
std::streambuf* pbasebuf2 = active_buffer(sstrm1);
- VERIFY( buf1 == buf2 );
+ VERIFY( buf1 == buf2 );
VERIFY( pbasebuf1 != pbasebuf2 );
VERIFY( pbasebuf2 == pbasebuf0 );
@@ -58,7 +58,11 @@ void test02()
VERIFY( sstrm1.str() != str01 );
VERIFY( sstrm1.str() == str00 );
// however, casting the active streambuf to a stringbuf shows what's up:
+#if __cpp_rtti
std::stringbuf* psbuf = dynamic_cast(pbasebuf2);
+#else
+ std::stringbuf* psbuf = static_cast(pbasebuf2);
+#endif
str02 = psbuf->str();
VERIFY( str02 == str01 );
diff --git
a/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/wchar_t/2832.cc
b/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/wchar_t/2832.cc
index b24ecc6619f..64475b5fc61 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/wchar_t/2832.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istringstream/rdbuf/wchar_t/2832.cc
@@ -20,8 +20,8 @@
#include
#include
-void
-redirect_buffer(std::wios& stream, std::wstreambuf* new_buf)
+void
+redirect_buffer(std::wios& stream, std::wstreambuf* new_buf)
{ stream.rdbuf(new_buf); }
std::wstreambuf*
@@ -48,7 +48,7 @@ void test02()
redirect_buffer(sstrm1, );
std::wstringbuf* const buf2 = sstrm1.rdbuf();
std::wstreambuf* pbasebuf2 = active_buffer(sstrm1);
- VERIFY( buf1 == buf2 );
+ VERIFY( buf1 == buf2 );
VERIFY( pbasebuf1 != pbasebuf2 );
VERIFY( pbasebuf2 == pbasebuf0 );
@@ -56,7 +56,11 @@ void test02()
VERIFY( sstrm1.str() != str01 );
VERIFY( sstrm1.str() == str00 );
// however, casting the active streambuf to a stringbuf shows what's up:
+#if __cpp_rtti
std::wstringbuf* psbuf = dynamic_cast(pbasebuf2);
+#else
+ std::wstringbuf* psbuf = static_cast(pbasebuf2);
+#endif
str02 = psbuf->str();
VERIFY( str02 == str01 );
diff --git
a/libstdc++-v3/testsuite/27_io/basic_ostringstream/rdbuf/char/2832.cc
b/libstdc++-v3/testsuite/27_io/basic_ostringstream/rdbuf/char/2832.cc
index 825aaf32cef..553edd6c243 100644
--- a/libstdc++-v3/testsuite/27_io/basic_ostringstream/rdbuf/char/2832.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_ostringstream/rdbuf/char/2832.cc
@@ -22,8 +22,8 @@
#include
#include
-void
-redirect_buffer(std::ios& stream, std::streambuf* new_buf)
+void
+redirect_buffer(std::ios& stream, std::streambuf* new_buf)
{ stream.rdbuf(new_buf); }
std::streambuf*
@@ -50,7 +50,7 @@ void test02()
redirect_buffer(sstrm1, );
std::stringbuf* const buf2 = sstrm1.rdbuf();
std::streambuf* pbasebuf2 = active_buffer(sstrm1);
- VERIFY( buf1 == buf2 );
+ VERIFY( buf1 == buf2 );
VERIFY( pbasebuf1 != pbasebuf2 );
VERIFY( pbasebuf2 == pbasebuf0 );
@@ -58,7 +58,11 @@ void test02()
VERIFY( sstrm1.str() != str01 );
VERIFY( sstrm1.str() == str00 );
// however, casting the active streambuf to a stringbuf shows what's up:
+#if __cpp_rtti
std::stringbuf* psbuf = dynamic_cast(pbasebuf2);
+#else
+
Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti
On 12/02/21 12:31 +0100, Mirko Vogt wrote:
On 2/12/21 11:30 AM, Jonathan Wakely wrote:
This patch is wrong.
Indeed, thanks for checking.
If you simply disable that function definition
for !__cpp_rtti then you'll get linker errors from fstream.tcc when
that function is called.
/usr/bin/ld:
/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so:
undefined reference to `std::__throw_ios_failure(char const*, int)'
Funny enough that doesn't occur for my use case - builds fine. However
building with a toolchain for bare metal, potentially resulting in
disabling e.g. fstream.
This was done correctly for the c++98 part and probably just forgotten
for c++11.
This has nothing to do with C++98, it's relted to the gcc4-compatible
ABI versus the cxx11 ABI.
Urgs, yeah, that last chunk for cxx98 expands a different macro to
include that definition - not the rtti ifdef. Misread and wrongly took
over.
I added a better patch to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99077
I've committed that patch to master now (and will backport it to
gcc-10 and gcc-9 soon). Thanks for finding the bug.
commit 4591f7e5329dcc6ee9af2f314a050936d470ab5b
Author: Jonathan Wakely
Date: Fri Feb 12 10:37:56 2021
libstdc++: Fix bootstrap with -fno-rtti [PR 99077]
When libstdc++ is built without RTTI the __ios_failure type is just an
alias for std::ios_failure, so trying to construct it from an int won't
compile. This changes the RTTI-enabled __ios_failure type to have the
same constructor parameters as std::ios_failure, so that the constructor
takes the same arguments whether RTTI is enabled or not.
The __throw_ios_failure function now constructs the error_code, instead
of the __ios_failure constructor. As a drive-by fix that error_code is
constructed with std::generic_category() not std::system_category(),
because the int comes from errno which corresponds to the generic
category.
libstdc++-v3/ChangeLog:
PR libstdc++/99077
* src/c++11/cxx11-ios_failure.cc (__ios_failure(const char*, int)):
Change int parameter to error_code, to match std::ios_failure.
(__throw_ios_failure(const char*, int)): Construct error_code
from int parameter.
diff --git a/libstdc++-v3/src/c++11/cxx11-ios_failure.cc b/libstdc++-v3/src/c++11/cxx11-ios_failure.cc
index e82c1aaf63b..a918ab21015 100644
--- a/libstdc++-v3/src/c++11/cxx11-ios_failure.cc
+++ b/libstdc++-v3/src/c++11/cxx11-ios_failure.cc
@@ -114,7 +114,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__ios_failure(const char* s) : failure(s)
{ __construct_ios_failure(buf, runtime_error::what()); }
-__ios_failure(const char* s, int e) : failure(s, to_error_code(e))
+__ios_failure(const char* s, const error_code& e) : failure(s, e)
{ __construct_ios_failure(buf, runtime_error::what()); }
~__ios_failure()
@@ -125,10 +125,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// There are assertions in src/c++98/ios_failure.cc to ensure the size
// and alignment assumptions are valid.
alignas(runtime_error) unsigned char buf[sizeof(runtime_error)];
-
-static error_code
-to_error_code(int e)
-{ return e ? error_code(e, system_category()) : io_errc::stream; }
};
// Custom type info for __ios_failure.
@@ -171,7 +167,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
void
__throw_ios_failure(const char* str __attribute__((unused)),
int err __attribute__((unused)))
- { _GLIBCXX_THROW_OR_ABORT(__ios_failure(_(str), err)); }
+ {
+_GLIBCXX_THROW_OR_ABORT(__ios_failure(_(str),
+ err ? error_code(err, generic_category()) : io_errc::stream));
+ }
_GLIBCXX_END_NAMESPACE_VERSION
} // namespace
Re: [PATCH] openmp: Fix intermittent hanging of task-detach-6 libgomp tests [PR98738]
On Fri, Jan 29, 2021 at 7:53 AM Jakub Jelinek via Gcc-patches
wrote:
>
> On Thu, Jan 21, 2021 at 07:33:34PM +, Kwok Cheung Yeung wrote:
> > The detach support clearly needs more work, but is this particular patch
> > okay for trunk?
>
> Sorry for the delay.
>
> I'm afraid it is far from being ready.
>
> > @@ -2402,17 +2437,41 @@ ialias (omp_in_final)
> > void
> > omp_fulfill_event (omp_event_handle_t event)
> > {
> > - gomp_sem_t *sem = (gomp_sem_t *) event;
> > + struct gomp_task *task = (struct gomp_task *) event;
> > + struct gomp_task *parent = task->parent;
> >struct gomp_thread *thr = gomp_thread ();
> >struct gomp_team *team = thr ? thr->ts.team : NULL;
> >
> > - if (gomp_sem_getcount (sem) > 0)
> > -gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", sem);
> > + if (gomp_sem_getcount (>completion_sem) > 0)
> > +gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", task);
>
> As written earlier, the intent of omp_fulfill_event is that it should be
> callable from anywhere, not necessarily one of the threads in the team.
> The application could have other threads (often called unshackeled threads)
> from which it would call it, or just some other parallel or whatever else,
> as long as it is not racy to pass in the omp_event_handle_t to there.
> So,
>struct gomp_thread *thr = gomp_thread ();
>struct gomp_team *team = thr ? thr->ts.team : NULL;
> is incorrect, it will give you the team of the current thread, rather than
> the team of the task to be fulfilled.
>
> It can also crash if team is NULL, which will happen any time
> this is called outside of a parallel. Just try (should go into testsuite
> too):
> #include
>
> int
> main ()
> {
> omp_event_handle_t ev;
> #pragma omp task detach (ev)
> omp_fulfill_event (ev);
> return 0;
> }
>
> Additionally, there is an important difference between fulfill for
> included tasks and for non-included tasks, for the former there is no team
> or anything to care about, for the latter there is a team and one needs to
> take the task_lock, but at that point it can do pretty much everything in
> omp_fulfill_event rather than handling it elsewhere.
>
> So, what I'm suggesting is:
>
> Replace
> bool detach;
> gomp_sem_t completion_sem;
> with
> struct gomp_task_detach *detach;
> and add struct gomp_task_detach that would contain everything that will be
> needed (indirect so that we don't waste space for it in every task, but only
> for those that have detach clause).
> We need:
> 1) some way to tell if it is an included task or not
> 2) for included tasks the gomp_sem_t completion_sem
> (and nothing but 1) and 2) for those),
> 3) struct gomp_team * for non-included tasks
> 4) some way to find out if the task has finished and is just waiting for
> fulfill event (perhaps your GOMP_TASK_DETACHED is ok for that)
> 5) some way to find out if the task has been fulfilled already
> (gomp_sem_t for that seems an overkill though)
>
> 1) could be done through the struct gomp_team *team; member,
> set it to NULL in included tasks (no matter if they are in some team or not)
> and to non-NULL team of the task (non-included tasks must have a team).
>
> And I don't see the point of task_detach_queue if we can handle the
> dependers etc. all in omp_fulfill_event, which I think we can if we take the
> task_lock.
>
> So, I think omp_fulfill_event should look at the task->detach it got,
> if task->detach->team is NULL, it is included task, GOMP_task should have
> initialized task->detach->completion_sem and omp_fulfill_event should just
> gomp_sem_post it and that is all, GOMP_task for included task needs to
> gomp_sem_wait after it finishes before it returns.
>
> Otherwise, take the team's task_lock, and look at whether the task is still
> running, in that case just set the bool that it has been fulfilled (or
> whatever way of signalling 5), perhaps it can be say clearing task->detach
> pointer). When creating non-included tasks in GOMP_task with detach clause
> through gomp_malloc, it would add the size needed for struct
> gomp_task_detach.
> But if the task is already in GOMP_TASK_DETACHED state, instead we need
> while holding the task_lock do everything that would have been done normally
> on task finish, but we've skipped it because it hasn't been fulfilled.
> Including the waking/sem_posts when something could be waiting on that task.
>
> Do you agree with this, or see some reason why this can't work?
>
> And testsuite should include also cases where we wait for the tasks with
> detach clause to be fulfilled at the end of taskgroup (i.e. need to cover
> all of taskwait, taskgroup end and barrier).
>
task-detach-6.f90 should be disabled for now. It has been blocking my testers
for weeks.
--
H.J.
[OBVIOUS][PATCH] testsuite, arm: Add -mthumb to pr98931.c [PR target/98931]
This test forces -march=armv8.1-m.main, which supports only Thumb mode.
However, if the toolchain is not configured --with-thumb, the test
fails with:
error: target CPU does not support ARM mode
Adding -mthumb to dg-options fixes the problem.
Committed as obvious.
2021-02-12 Christophe Lyon
PR target/98931
gcc/testsuite/
* gcc.target/arm/pr98931.c: Add -mthumb
---
gcc/testsuite/gcc.target/arm/pr98931.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/testsuite/gcc.target/arm/pr98931.c
b/gcc/testsuite/gcc.target/arm/pr98931.c
index 313876a..66070ad 100644
--- a/gcc/testsuite/gcc.target/arm/pr98931.c
+++ b/gcc/testsuite/gcc.target/arm/pr98931.c
@@ -1,6 +1,6 @@
/* { dg-do assemble } */
/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm"
"-mcpu=*" } } */
-/* { dg-options "-march=armv8.1-m.main -O3
--param=max-completely-peeled-insns=1300 --save-temps" } */
+/* { dg-options "-march=armv8.1-m.main -O3
--param=max-completely-peeled-insns=1300 --save-temps -mthumb" } */
extern long long a[][20][26][26][22];
--
2.7.4
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
On 2021-01-11 01:01 +0800, Xi Ruoyao wrote: > Hi Jeff and Jakub, > > On 2021-01-04 14:19 -0700, Jeff Law wrote: > > On 1/4/21 2:00 PM, Jakub Jelinek wrote: > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches wrote: > > > > > Sorry, I forgot to include the ChangeLog: > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > 2020-12-31 Xi Ruoyao > > > > > > > > > > PR target/98491 > > > > > * config/mips/mips.c (mips_symbol_insns): Do not use > > > > > MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE. > > > > So I absolutely agree the current code is wrong as it does an out of > > > > bounds array access. > > > > > > > > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate > > > > to zero if MODE is MAX_MACHINE_MODE? That would protect all the uses of > > > > MSA_SUPPORTED_MODE_P. Something like this perhaps? > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of > > > any target that would protect all macros that deal with modes that way. > > > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value > > > for that function and instead use say VOIDmode that shouldn't normally > > > appear either? > > I think we have to allow VOIDmode because constants don't necessarily > > have modes. And I certainly agree that using MAX_MACHINE_MODE like > > this is ugly and error prone (as we can see from the BZ). > > > > I also couldn't convince myself that the code and comments were actually > > consistent, particularly for MSA targets which the comment claims can > > never handle constants for ld/st (and thus should be returning 0 for > > MAX_MACHINE_MODE). Though maybe mips_symbol_insns_1 ultimately handles > > that correctly. > > > > > > > > > > But I don't really see anything wrong on the mips_symbol_insns above > > > change either. > > Me neither. I'm just questioning if bullet-proofing in the > > MSA_SUPPORTED_MODE_P would be a better option. While I've worked in the > > MIPS port in the past, I don't really have any significannt experience > > with the MSA support. > > I can't understand the comment either. To me it looks like it's possible to > remove this "if (MSA_SUPPORTED_P (mode)) return 0;" > > CC Robert to get some help. Happy new lunar year folks. I found a newer email address of Robert. Hope it is still being used. Could someone update MAINTAINERS file by the way? -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
Re: [Patch, fortran] PR98897 - Erroneous procedure attribute for associate name
Hi All,
Following off-list discussion with Tobias, I have committed the patch as
submitted to 10- and 11-branches.
A rather general problem with parsing and matching, which arose from the
discussion, has been shunted into PR99065. If possible, I intend to fix
this by two pass parsing/matching of all contained procedures; first all
specification blocks and then, on the second pass, the code blocks. This
should eliminate the need for the likes of
parse.c(gfc_fixup_sibling_symbols) and some similar fixups in resolve.c.
Regards
Paul
On Tue, 2 Feb 2021 at 15:56, Tobias Burnus wrote:
> Hi,
>
> first, I have attached a new example – it works if I move bar/hello up,
> but if 'foo' comes first, it fails. I think it is valid.
> (ifort 19 also compiles it.)
>
> Sorry for trying hard to find examples where it does not
> work – but I have simply the feeling that resolving things
> during parsing cannot work in all cases.
>
> On the other hand, I think your patch at least does not break
> valid code as I had feared before. :-)
> Thus, in that sense it would work for me.
>
> * * *
>
> Regarding my previous examples, they are invalid because of:
>
> C1105 (R1105) expr shall not be a designator of a procedure pointer
> or a function reference that returns a procedure pointer.
>
> However:
>
> On 02.02.21 16:05, Paul Richard Thomas via Fortran wrote:
>
> > In foo.f90, if I remove
> > call var(i) ! { dg-bogus "VARIABLE attribute of 'var' conflicts
> with
> > PROCEDURE attribute" }
> > gfortran correctly complains
> > 23 | associate (var => bar())
> >| 1
> > Error: Selector at (1) has no type
>
> Which is not quite right. bar() has a type – it returns
> a procedure pointer; even in cases where gfortran could
> know at parse time, it does not diagnose C1105 but shows
> an odd error instead.
>
> > ifort complains:
> > ../pr98897/foo.f90(11): error #8179: The procedure pointer and the
> > procedure target must both be functions or subroutines.
> > res => double
> Okay, we found a bug in ifort. 'double' and 'res' have the same
> interface by construction – and both are subroutines.
> It seems to be a similar bug to the ifort bug I got before:
> When 'double' is parsed, ifort expects that 'precision' follows
> ('double precision').
>
> > The responses from both compilers to foo3.f90 are the same.
>
> (I forgot to comment/remove 'procedure(bar) :: var' when
> playing around.) Again, this code violates C1105 – and the
> error messages are still odd.
>
> > On Tue, 2 Feb 2021 at 13:59, Tobias Burnus
> wrote:
> > On 02.02.21 13:20, Paul Richard Thomas via Gcc-patches wrote:
> >>> Regtests with FC33/x86_64 - OK for master (and )?
> >>> Fortran: Fix calls to associate name typebound subroutines [PR98897].
> >>>
> >>> 2021-02-02 Paul Thomas
> >>>
> >>> gcc/fortran
> >>> PR fortran/98897
> >>> * match.c (gfc_match_call): Include associate names as possible
> >>> entities with typebound subroutines. The target needs to be
> >>> resolved for the type.
> >>>
> >>> gcc/testsuite/
> >>> PR fortran/98897
> >>> * gfortran.dg/typebound_call_32.f90: New test.
> -
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
> Thürauf
>
--
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein
Re: [Patch] Fortran: Fix rank of assumed-rank array [PR99043]
Hi Tobias, The patch is good for 10- and 11-branches. Thanks Paul On Thu, 11 Feb 2021 at 18:59, Tobias Burnus wrote: > In the Fortran standard, I think it is best explained > in the description of the RANK intrinsic: > > "Example. If X is an assumed-rank dummy argument and > its associated effective argument is an array of rank, > RANK(X) has the value 3." > > That's already well tested in assumed_rank_16.f90; > however, as the PR shows, this should not be reset > to "-1" when passing it further on as actual argument to > another assumed-rank dummy argument. > > OK for mainline? > Reported against GCC 10, not a regression but simple wrong-code fix; > does it make sense to apply there was well? > > Tobias > > - > Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München > Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank > Thürauf > -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
Re: [RFC] test builtin ratio for loop distribution
On Thu, Feb 11, 2021 at 11:19 AM Alexandre Oliva wrote: > > On Feb 4, 2021, Alexandre Oliva wrote: > > > On Feb 4, 2021, Richard Biener wrote: > >>> > b) if expansion would use BY_PIECES then expand to an unrolled loop > >>> > >>> Why would that be better than keeping the constant-length memset call, > >>> that would be turned into an unrolled loop during expand? > > >> Well, because of the possibly lost ctz and alignment info. > > > Funny you should mention that. I got started with the expand-time > > expansion yesterday, and found out that we're not using the alignment > > information that is available. Though the pointer is known to point to > > an aligned object, we are going for 8-bit alignment for some reason. > > > The strategy I used there was to first check whether by_pieces would > > expand inline a constant length near the max known length, then loop > > over the bits in the variable length, expand in each iteration a > > constant-length store-by-pieces for the fixed length corresponding to > > that bit, and a test comparing the variable length with the fixed length > > guarding the expansion of the store-by-pieces. We may get larger code > > this way (no loops), but only O(log(len)) compares. > > > I've also fixed some bugs in the ldist expander, so now it bootstraps, > > but with a few regressions in the testsuite, that I'm yet to look into. > > A few more fixes later, this seems to work. > > It encompasses the patch to recover tree_ctz from a mult_expr def, it > adds code to set up the alignment data for the ldist-generated memset > dest, and then it expands memset as described above if length is not > constant, setmem is not available, but the by-pieces machinery would > still be used for nearby constants. > > How does this look? Overall it looks good - I can't really review the RTL code generation parts because I'm not too familiar with the usual pitfalls there. Some comments below. > > [PR94092] introduce try store by multiple pieces > > From: Alexandre Oliva > > The ldist pass turns even very short loops into memset calls. E.g., > the TFmode emulation calls end with a loop of up to 3 iterations, to > zero out trailing words, and the loop distribution pass turns them > into calls of the memset builtin. > > Though short constant-length memsets are usually dealt with > efficiently, for non-constant-length ones, the options are setmemM, or > a function calls. > > RISC-V doesn't have any setmemM pattern, so the loops above end up > "optimized" into memset calls, incurring not only the overhead of an > explicit call, but also discarding the information the compiler has > about the alignment of the destination, and that the length is a > multiple of the word alignment. > > This patch handles variable lengths with multiple conditional > power-of-2-constant-sized stores-by-pieces, so as to reduce the > overhead of length compares. > > It also propagates the alignment of the memset dest, introduced by > loop-distribution, to the SSA pointer used to hold it, so that it is > not discarded right away, and may be recovered by the memset builtin > expander. > > Finally, it computes the ctz of the length, and uses it to omit blocks > for stores of small byte counts when we're storing whole words. > tree_ctz is improved so as to look at the length DEF stmt, and zero > out the least-significant bits if it's a multiply by a power of two. > > > for gcc/ChangeLog > > PR tree-optimization/94092 > * builtins.c (try_store_by_multiple_pieces): New. > (expand_builtin_memset_args): Use it. If target_char_cast > fails, proceed as for non-constant val. Pass len's ctz to... > * expr.c (clear_storage_hints): ... this. Try store by > multiple pieces after setmem. > (clear_storage): Adjust. > * expr.h (clear_storage_hints): Likewise. > (try_store_by_multiple_pieces): Declare. > * tree-loop-distribution.c: Include builtins.h. > (generate_memset_builtin): Propagate dst_base alignmen to mem. > * tree-ssanames.c (get_nonzero_bits): Zero out low bits of > integral types, when a MULT_EXPR INTEGER_CST operand ensures > the result will be a multiple of a power of two. > --- > gcc/builtins.c | 113 > +++--- > gcc/expr.c |9 +++ > gcc/expr.h | 12 > gcc/tree-loop-distribution.c |9 +++ > gcc/tree-ssanames.c | 23 - > 5 files changed, 154 insertions(+), 12 deletions(-) > > diff --git a/gcc/builtins.c b/gcc/builtins.c > index 0aed008687ccc..44f3af92536a5 100644 > --- a/gcc/builtins.c > +++ b/gcc/builtins.c > @@ -6600,6 +6600,97 @@ expand_builtin_memset (tree exp, rtx target, > machine_mode mode) >return expand_builtin_memset_args (dest, val, len, target, mode, exp); > } > > +/* Try to store VAL (or, if NULL_RTX, VALC) in LEN bytes starting at TO. > + Return TRUE if
Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti
On 2/12/21 11:30 AM, Jonathan Wakely wrote: This patch is wrong. Indeed, thanks for checking. If you simply disable that function definition for !__cpp_rtti then you'll get linker errors from fstream.tcc when that function is called. /usr/bin/ld: /home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `std::__throw_ios_failure(char const*, int)' Funny enough that doesn't occur for my use case - builds fine. However building with a toolchain for bare metal, potentially resulting in disabling e.g. fstream. This was done correctly for the c++98 part and probably just forgotten for c++11. This has nothing to do with C++98, it's relted to the gcc4-compatible ABI versus the cxx11 ABI. Urgs, yeah, that last chunk for cxx98 expands a different macro to include that definition - not the rtti ifdef. Misread and wrongly took over. I added a better patch to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99077 Thanks! PS: Shouldn't this have been covered by any tests? Nobody tests building libstdc++ with -fno-rtti because almost nobody does that. Just as info: Espressif's crosstool-ng fork (not sure about vanilla) is building with no-rtti for targeting their Xtensa based ESP32 SoCs. That's how I stumbled over this. But we have plenty of tests, and hundreds of them fail with your patch :-) Yeah, glass houses and stones.. sorry for that :) mirko
[PATCH] middle-end/38474 - fix alias walk budget accounting in IPA analysis
The walk_aliased_vdef calls do not update the walking budget until
it is hit by a single call (and then in one case it resumes with
no limit at all). The following rectifies this in multiple places.
It also makes the updates more consistend and fixes
determine_known_aggregate_parts to account its own alias queries.
Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
2021-02-12 Richard Biener
PR middle-end/38474
* ipa-fnsummary.c (unmodified_parm_1): Only walk when
fbi->aa_walk_budget is bigger than zero. Update
fbi->aa_walk_budget.
(param_change_prob): Likewise.
* ipa-prop.c (detect_type_change_from_memory_writes):
Properly account walk_aliased_vdefs.
(parm_preserved_before_stmt_p): Canonicalize updates.
(parm_ref_data_preserved_p): Likewise.
(parm_ref_data_pass_through_p): Likewise.
(determine_known_aggregate_parts): Account own alias queries.
---
gcc/ipa-fnsummary.c | 12 +---
gcc/ipa-prop.c | 30 +++---
2 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 70b1fc25edb..e32e69cd3ad 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -1197,7 +1197,8 @@ unmodified_parm_1 (ipa_func_body_info *fbi, gimple *stmt,
tree op,
return SSA_NAME_VAR (op);
}
/* Non-SSA parm reference? */
- if (TREE_CODE (op) == PARM_DECL)
+ if (TREE_CODE (op) == PARM_DECL
+ && fbi->aa_walk_budget > 0)
{
bool modified = false;
@@ -1205,12 +1206,13 @@ unmodified_parm_1 (ipa_func_body_info *fbi, gimple
*stmt, tree op,
ao_ref_init (, op);
int walked = walk_aliased_vdefs (, gimple_vuse (stmt),
mark_modified, , NULL, NULL,
- fbi->aa_walk_budget + 1);
+ fbi->aa_walk_budget);
if (walked < 0)
{
fbi->aa_walk_budget = 0;
return NULL_TREE;
}
+ fbi->aa_walk_budget -= walked;
if (!modified)
{
if (size_p)
@@ -2240,7 +2242,7 @@ param_change_prob (ipa_func_body_info *fbi, gimple *stmt,
int i)
if (init != error_mark_node)
return 0;
- if (!bb->count.nonzero_p ())
+ if (!bb->count.nonzero_p () || fbi->aa_walk_budget == 0)
return REG_BR_PROB_BASE;
if (dump_file)
{
@@ -2255,8 +2257,12 @@ param_change_prob (ipa_func_body_info *fbi, gimple
*stmt, int i)
int walked
= walk_aliased_vdefs (, gimple_vuse (stmt), record_modified, ,
NULL, NULL, fbi->aa_walk_budget);
+ if (walked > 0)
+ fbi->aa_walk_budget -= walked;
if (walked < 0 || bitmap_bit_p (info.bb_set, bb->index))
{
+ if (walked < 0)
+ fbi->aa_walk_budget = 0;
if (dump_file)
{
if (walked < 0)
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 9e348990dc9..010c43f33e8 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -803,6 +803,9 @@ detect_type_change_from_memory_writes (ipa_func_body_info
*fbi, tree arg,
|| !BINFO_VTABLE (TYPE_BINFO (TYPE_MAIN_VARIANT (comp_type
return true;
+ if (fbi->aa_walk_budget == 0)
+return false;
+
ao_ref_init (, arg);
ao.base = base;
ao.offset = offset;
@@ -815,7 +818,11 @@ detect_type_change_from_memory_writes (ipa_func_body_info
*fbi, tree arg,
int walked
= walk_aliased_vdefs (, gimple_vuse (call), check_stmt_for_type_change,
- , NULL, NULL, fbi->aa_walk_budget + 1);
+ , NULL, NULL, fbi->aa_walk_budget);
+ if (walked >= 0)
+fbi->aa_walk_budget -= walked;
+ else
+fbi->aa_walk_budget = 0;
if (walked >= 0 && !tci.type_maybe_changed)
return false;
@@ -948,21 +955,20 @@ parm_preserved_before_stmt_p (struct ipa_func_body_info
*fbi, int index,
gcc_checking_assert (fbi);
paa = parm_bb_aa_status_for_bb (fbi, gimple_bb (stmt), index);
- if (paa->parm_modified)
+ if (paa->parm_modified || fbi->aa_walk_budget == 0)
return false;
gcc_checking_assert (gimple_vuse (stmt) != NULL_TREE);
ao_ref_init (, parm_load);
int walked = walk_aliased_vdefs (, gimple_vuse (stmt), mark_modified,
, NULL, NULL,
- fbi->aa_walk_budget + 1);
+ fbi->aa_walk_budget);
if (walked < 0)
{
modified = true;
- if (fbi)
- fbi->aa_walk_budget = 0;
+ fbi->aa_walk_budget = 0;
}
- else if (fbi)
+ else
fbi->aa_walk_budget -= walked;
if (paa && modified)
paa->parm_modified = true;
@@ -1010,14 +1016,14 @@ parm_ref_data_preserved_p (struct ipa_func_body_info
*fbi,
gcc_checking_assert (fbi);
paa = parm_bb_aa_status_for_bb (fbi, gimple_bb (stmt), index);
- if (paa->ref_modified)
+ if (paa->ref_modified ||
Re: [PATCH] df: Record all definitions in DF_LR_BB_INFO->def [PR98863]
On Fri, Feb 12, 2021 at 10:59 AM Richard Sandiford
wrote:
>
> Richard Biener writes:
> > On Thu, Feb 11, 2021 at 4:33 PM Richard Sandiford via Gcc-patches
> > wrote:
> >>
> >> df_lr_bb_local_compute has:
> >>
> >> FOR_EACH_INSN_INFO_DEF (def, insn_info)
> >> /* If the def is to only part of the reg, it does
> >>not kill the other defs that reach here. */
> >> if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
> >>
> >> However, as noted in the comment in the patch and below, almost all
> >> partial definitions have an associated use. This means that the
> >> confluence function:
> >>
> >> IN = (OUT & ~DEF) | USE
> >>
> >> is unaffected by whether partial definitions are in DEF or not.
> >>
> >> Even though the choice doesn't matter for the LR problem itself,
> >> it's IMO much more convenient for consumers if DEF contains all the
> >> definitions in the block. The only pre-RTL-SSA code that tries to
> >> consume DEF directly is shrink-wrap.c, which already has to work
> >> around the incompleteness of the information:
> >>
> >> /* DF_LR_BB_INFO (bb)->def does not comprise the DF_REF_PARTIAL
> >> and
> >> DF_REF_CONDITIONAL defs. So if DF_LIVE doesn't exist, i.e.
> >> at -O1, just give up searching NEXT_BLOCK. */
> >>
> >> I hit the same problem when trying to fix the RTL-SSA part of PR98863.
> >>
> >> This patch treats partial definitions as both a def and a use,
> >> just like the df_ref records almost always do.
> >>
> >> To show that partial definitions almost always have uses:
> >>
> >> DF_REF_CONDITIONAL:
> >>
> >> Added by:
> >>
> >> case COND_EXEC:
> >> df_defs_record (collection_rec, COND_EXEC_CODE (x),
> >> bb, insn_info, DF_REF_CONDITIONAL);
> >> break;
> >>
> >> Later, df_get_conditional_uses creates uses for all DF_REF_CONDITIONAL
> >> definitions.
> >>
> >> DF_REF_PARTIAL:
> >>
> >> In total, there are 4 locations at which we add partial definitions.
> >>
> >> Case 1:
> >>
> >> if (GET_CODE (dst) == STRICT_LOW_PART)
> >> {
> >> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL |
> >> DF_REF_STRICT_LOW_PART;
> >>
> >> loc = (dst, 0);
> >> dst = *loc;
> >> }
> >>
> >> Corresponding use:
> >>
> >> case STRICT_LOW_PART:
> >> {
> >> rtx *temp = (dst, 0);
> >> /* A strict_low_part uses the whole REG and not just the
> >> SUBREG. */
> >> dst = XEXP (dst, 0);
> >> df_uses_record (collection_rec,
> >> (GET_CODE (dst) == SUBREG) ? _REG (dst) :
> >> temp,
> >> DF_REF_REG_USE, bb, insn_info,
> >> DF_REF_READ_WRITE | DF_REF_STRICT_LOW_PART);
> >> }
> >> break;
> >>
> >> Case 2:
> >>
> >> if (GET_CODE (dst) == ZERO_EXTRACT)
> >> {
> >> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL |
> >> DF_REF_ZERO_EXTRACT;
> >>
> >> loc = (dst, 0);
> >> dst = *loc;
> >> }
> >>
> >> Corresponding use:
> >>
> >> case ZERO_EXTRACT:
> >> {
> >> df_uses_record (collection_rec, (dst, 1),
> >> DF_REF_REG_USE, bb, insn_info, flags);
> >> df_uses_record (collection_rec, (dst, 2),
> >> DF_REF_REG_USE, bb, insn_info, flags);
> >> if (GET_CODE (XEXP (dst,0)) == MEM)
> >> df_uses_record (collection_rec, (dst, 0),
> >> DF_REF_REG_USE, bb, insn_info,
> >> flags);
> >> else
> >> df_uses_record (collection_rec, (dst, 0),
> >> DF_REF_REG_USE, bb, insn_info,
> >> DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT);
> >> ^
> >> }
> >> break;
> >>
> >> Case 3:
> >>
> >> else if (GET_CODE (dst) == SUBREG && REG_P (SUBREG_REG (dst)))
> >> {
> >> if (read_modify_subreg_p (dst))
> >> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL;
> >>
> >> flags |= DF_REF_SUBREG;
> >>
> >> df_ref_record (DF_REF_REGULAR, collection_rec,
> >> dst, loc, bb, insn_info, DF_REF_REG_DEF, flags);
> >> }
> >>
> >> Corresponding use:
> >>
> >> case SUBREG:
> >> if (read_modify_subreg_p (dst))
> >> {
> >> df_uses_record (collection_rec, _REG (dst),
> >> DF_REF_REG_USE, bb, insn_info,
> >> flags | DF_REF_READ_WRITE | DF_REF_SUBREG);
> >> break;
> >> }
> >>
> >> Case 4:
> >>
> >> /* If this is a multiword hardreg, we create some extra
> >> datastructures that will enable us to easily build REG_DEAD
> >> and
Re: [PATCH] libstdc++: ifdef rtti specific function __throw_ios_failure() by __cpp_rtti
>Hello, > >ran into the following when building libstdc++ without rtti support: > >libstdc++-v3/src/c++11/cxx11-ios_failure.cc:174:54: error: no matching >function for call to 'std::ios_base::failure::failure(const char*&, int&)' > >Attached patch does as follows: Libstdc++ patches need to be CC'd to the libstdc++@ list as well. >ifdef rtti specific function __throw_ios_failure() by __cpp_rtti > >Overloaded __throw_ios_failure(const char*, int) got introduced in >484e936e88e5, however __ios_failure() with respective signature is only >defined if __cpp_rtti is defined, hence should only be used within >contexts also guarded by __cpp_rtti. This patch is wrong. If you simply disable that function definition for !__cpp_rtti then you'll get linker errors from fstream.tcc when that function is called. /usr/bin/ld: /home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `std::__throw_ios_failure(char const*, int)' >This was done correctly for the c++98 part and probably just forgotten >for c++11. This has nothing to do with C++98, it's relted to the gcc4-compatible ABI versus the cxx11 ABI. I added a better patch to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99077 >Thanks > > mirko > >PS: Shouldn't this have been covered by any tests? Nobody tests building libstdc++ with -fno-rtti because almost nobody does that. But we have plenty of tests, and hundreds of them fail with your patch :-)
Re: [PATCH] df: Record all definitions in DF_LR_BB_INFO->def [PR98863]
Richard Biener writes:
> On Thu, Feb 11, 2021 at 4:33 PM Richard Sandiford via Gcc-patches
> wrote:
>>
>> df_lr_bb_local_compute has:
>>
>> FOR_EACH_INSN_INFO_DEF (def, insn_info)
>> /* If the def is to only part of the reg, it does
>>not kill the other defs that reach here. */
>> if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
>>
>> However, as noted in the comment in the patch and below, almost all
>> partial definitions have an associated use. This means that the
>> confluence function:
>>
>> IN = (OUT & ~DEF) | USE
>>
>> is unaffected by whether partial definitions are in DEF or not.
>>
>> Even though the choice doesn't matter for the LR problem itself,
>> it's IMO much more convenient for consumers if DEF contains all the
>> definitions in the block. The only pre-RTL-SSA code that tries to
>> consume DEF directly is shrink-wrap.c, which already has to work
>> around the incompleteness of the information:
>>
>> /* DF_LR_BB_INFO (bb)->def does not comprise the DF_REF_PARTIAL and
>> DF_REF_CONDITIONAL defs. So if DF_LIVE doesn't exist, i.e.
>> at -O1, just give up searching NEXT_BLOCK. */
>>
>> I hit the same problem when trying to fix the RTL-SSA part of PR98863.
>>
>> This patch treats partial definitions as both a def and a use,
>> just like the df_ref records almost always do.
>>
>> To show that partial definitions almost always have uses:
>>
>> DF_REF_CONDITIONAL:
>>
>> Added by:
>>
>> case COND_EXEC:
>> df_defs_record (collection_rec, COND_EXEC_CODE (x),
>> bb, insn_info, DF_REF_CONDITIONAL);
>> break;
>>
>> Later, df_get_conditional_uses creates uses for all DF_REF_CONDITIONAL
>> definitions.
>>
>> DF_REF_PARTIAL:
>>
>> In total, there are 4 locations at which we add partial definitions.
>>
>> Case 1:
>>
>> if (GET_CODE (dst) == STRICT_LOW_PART)
>> {
>> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL |
>> DF_REF_STRICT_LOW_PART;
>>
>> loc = (dst, 0);
>> dst = *loc;
>> }
>>
>> Corresponding use:
>>
>> case STRICT_LOW_PART:
>> {
>> rtx *temp = (dst, 0);
>> /* A strict_low_part uses the whole REG and not just the
>> SUBREG. */
>> dst = XEXP (dst, 0);
>> df_uses_record (collection_rec,
>> (GET_CODE (dst) == SUBREG) ? _REG (dst) :
>> temp,
>> DF_REF_REG_USE, bb, insn_info,
>> DF_REF_READ_WRITE | DF_REF_STRICT_LOW_PART);
>> }
>> break;
>>
>> Case 2:
>>
>> if (GET_CODE (dst) == ZERO_EXTRACT)
>> {
>> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL | DF_REF_ZERO_EXTRACT;
>>
>> loc = (dst, 0);
>> dst = *loc;
>> }
>>
>> Corresponding use:
>>
>> case ZERO_EXTRACT:
>> {
>> df_uses_record (collection_rec, (dst, 1),
>> DF_REF_REG_USE, bb, insn_info, flags);
>> df_uses_record (collection_rec, (dst, 2),
>> DF_REF_REG_USE, bb, insn_info, flags);
>> if (GET_CODE (XEXP (dst,0)) == MEM)
>> df_uses_record (collection_rec, (dst, 0),
>> DF_REF_REG_USE, bb, insn_info,
>> flags);
>> else
>> df_uses_record (collection_rec, (dst, 0),
>> DF_REF_REG_USE, bb, insn_info,
>> DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT);
>> ^
>> }
>> break;
>>
>> Case 3:
>>
>> else if (GET_CODE (dst) == SUBREG && REG_P (SUBREG_REG (dst)))
>> {
>> if (read_modify_subreg_p (dst))
>> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL;
>>
>> flags |= DF_REF_SUBREG;
>>
>> df_ref_record (DF_REF_REGULAR, collection_rec,
>> dst, loc, bb, insn_info, DF_REF_REG_DEF, flags);
>> }
>>
>> Corresponding use:
>>
>> case SUBREG:
>> if (read_modify_subreg_p (dst))
>> {
>> df_uses_record (collection_rec, _REG (dst),
>> DF_REF_REG_USE, bb, insn_info,
>> flags | DF_REF_READ_WRITE | DF_REF_SUBREG);
>> break;
>> }
>>
>> Case 4:
>>
>> /* If this is a multiword hardreg, we create some extra
>> datastructures that will enable us to easily build REG_DEAD
>> and REG_UNUSED notes. */
>> if (collection_rec
>> && (endregno != regno + 1) && insn_info)
>> {
>> /* Sets to a subreg of a multiword register are partial.
>> Sets to a non-subreg of a multiword register are not. */
>> if (GET_CODE (reg) == SUBREG)
>> ref_flags |= DF_REF_PARTIAL;
>>
[PATCH] arm: Fix ICE with -fstack-protector -mpure-code [PR98998]
Hi!
The vla15.C testcase ICEs with
-mcpu=cortex-m1 -mpure-code -fstack-protector -mthumb
as what force_const_mem returns (a SYMBOL_REF) is not a valid
memory address.
Previously the code was moving the address of the force_const_mem
into a register rather than the content of that MEM, so that instruction
must have been supported and loading from a MEM with a single REG base ought
to be valid too.
Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk?
No testcase, as I haven't figured out my way through gcc.target/arm, sorry.
2021-02-12 Jakub Jelinek
PR target/98998
* config/arm/arm.md (*stack_protect_combined_set_insn,
*stack_protect_combined_test_insn): If force_const_mem result
is not valid general operand, force its address into the destination
register first.
--- gcc/config/arm/arm.md.jj2021-01-04 10:25:44.404170742 +0100
+++ gcc/config/arm/arm.md 2021-02-11 12:50:26.049604711 +0100
@@ -9216,6 +9216,11 @@ (define_insn_and_split "*stack_protect_c
else
{
rtx mem = force_const_mem (SImode, operands[1]);
+ if (!general_operand (mem, SImode))
+ {
+ emit_move_insn (operands[2], XEXP (mem, 0));
+ mem = replace_equiv_address (mem, operands[2], false);
+ }
emit_move_insn (operands[2], mem);
}
}
@@ -9299,6 +9304,11 @@ (define_insn_and_split "*stack_protect_c
else
{
rtx mem = force_const_mem (SImode, operands[1]);
+ if (!general_operand (mem, SImode))
+ {
+ emit_move_insn (operands[3], XEXP (mem, 0));
+ mem = replace_equiv_address (mem, operands[3], false);
+ }
emit_move_insn (operands[3], mem);
}
}
Jakub
Re: [PATCH] Fix producer string memory leaks
On Thu, Feb 11, 2021 at 6:41 PM Martin Liška wrote:
>
> Hello.
>
> This fixes 2 memory leaks I noticed.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
OK.
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> * opts-common.c (decode_cmdline_option): Release werror_arg.
> * opts.c (gen_producer_string): Release output of
> gen_command_line_string.
> ---
> gcc/opts-common.c | 1 +
> gcc/opts.c| 7 +--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/opts-common.c b/gcc/opts-common.c
> index 6cb5602896d..5e10edeb4cf 100644
> --- a/gcc/opts-common.c
> +++ b/gcc/opts-common.c
> @@ -766,6 +766,7 @@ decode_cmdline_option (const char *const *argv, unsigned
> int lang_mask,
> werror_arg[0] = 'W';
>
> size_t warning_index = find_opt (werror_arg, lang_mask);
> + free (werror_arg);
> if (warning_index != OPT_SPECIAL_unknown)
> {
> const struct cl_option *warning_option
> diff --git a/gcc/opts.c b/gcc/opts.c
> index fc5f61e13cc..24bb64198c8 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -3401,8 +3401,11 @@ char *
> gen_producer_string (const char *language_string, cl_decoded_option
> *options,
> unsigned int options_count)
> {
> - return concat (language_string, " ", version_string, " ",
> -gen_command_line_string (options, options_count), NULL);
> + char *cmdline = gen_command_line_string (options, options_count);
> + char *combined = concat (language_string, " ", version_string, " ",
> + cmdline, NULL);
> + free (cmdline);
> + return combined;
> }
>
> #if CHECKING_P
> --
> 2.30.0
>
Re: [PATCH] df: Record all definitions in DF_LR_BB_INFO->def [PR98863]
On Thu, Feb 11, 2021 at 4:33 PM Richard Sandiford via Gcc-patches
wrote:
>
> df_lr_bb_local_compute has:
>
> FOR_EACH_INSN_INFO_DEF (def, insn_info)
> /* If the def is to only part of the reg, it does
>not kill the other defs that reach here. */
> if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
>
> However, as noted in the comment in the patch and below, almost all
> partial definitions have an associated use. This means that the
> confluence function:
>
> IN = (OUT & ~DEF) | USE
>
> is unaffected by whether partial definitions are in DEF or not.
>
> Even though the choice doesn't matter for the LR problem itself,
> it's IMO much more convenient for consumers if DEF contains all the
> definitions in the block. The only pre-RTL-SSA code that tries to
> consume DEF directly is shrink-wrap.c, which already has to work
> around the incompleteness of the information:
>
> /* DF_LR_BB_INFO (bb)->def does not comprise the DF_REF_PARTIAL and
> DF_REF_CONDITIONAL defs. So if DF_LIVE doesn't exist, i.e.
> at -O1, just give up searching NEXT_BLOCK. */
>
> I hit the same problem when trying to fix the RTL-SSA part of PR98863.
>
> This patch treats partial definitions as both a def and a use,
> just like the df_ref records almost always do.
>
> To show that partial definitions almost always have uses:
>
> DF_REF_CONDITIONAL:
>
> Added by:
>
> case COND_EXEC:
> df_defs_record (collection_rec, COND_EXEC_CODE (x),
> bb, insn_info, DF_REF_CONDITIONAL);
> break;
>
> Later, df_get_conditional_uses creates uses for all DF_REF_CONDITIONAL
> definitions.
>
> DF_REF_PARTIAL:
>
> In total, there are 4 locations at which we add partial definitions.
>
> Case 1:
>
> if (GET_CODE (dst) == STRICT_LOW_PART)
> {
> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL |
> DF_REF_STRICT_LOW_PART;
>
> loc = (dst, 0);
> dst = *loc;
> }
>
> Corresponding use:
>
> case STRICT_LOW_PART:
> {
> rtx *temp = (dst, 0);
> /* A strict_low_part uses the whole REG and not just the
> SUBREG. */
> dst = XEXP (dst, 0);
> df_uses_record (collection_rec,
> (GET_CODE (dst) == SUBREG) ? _REG (dst) :
> temp,
> DF_REF_REG_USE, bb, insn_info,
> DF_REF_READ_WRITE | DF_REF_STRICT_LOW_PART);
> }
> break;
>
> Case 2:
>
> if (GET_CODE (dst) == ZERO_EXTRACT)
> {
> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL | DF_REF_ZERO_EXTRACT;
>
> loc = (dst, 0);
> dst = *loc;
> }
>
> Corresponding use:
>
> case ZERO_EXTRACT:
> {
> df_uses_record (collection_rec, (dst, 1),
> DF_REF_REG_USE, bb, insn_info, flags);
> df_uses_record (collection_rec, (dst, 2),
> DF_REF_REG_USE, bb, insn_info, flags);
> if (GET_CODE (XEXP (dst,0)) == MEM)
> df_uses_record (collection_rec, (dst, 0),
> DF_REF_REG_USE, bb, insn_info,
> flags);
> else
> df_uses_record (collection_rec, (dst, 0),
> DF_REF_REG_USE, bb, insn_info,
> DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT);
> ^
> }
> break;
>
> Case 3:
>
> else if (GET_CODE (dst) == SUBREG && REG_P (SUBREG_REG (dst)))
> {
> if (read_modify_subreg_p (dst))
> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL;
>
> flags |= DF_REF_SUBREG;
>
> df_ref_record (DF_REF_REGULAR, collection_rec,
> dst, loc, bb, insn_info, DF_REF_REG_DEF, flags);
> }
>
> Corresponding use:
>
> case SUBREG:
> if (read_modify_subreg_p (dst))
> {
> df_uses_record (collection_rec, _REG (dst),
> DF_REF_REG_USE, bb, insn_info,
> flags | DF_REF_READ_WRITE | DF_REF_SUBREG);
> break;
> }
>
> Case 4:
>
> /* If this is a multiword hardreg, we create some extra
> datastructures that will enable us to easily build REG_DEAD
> and REG_UNUSED notes. */
> if (collection_rec
> && (endregno != regno + 1) && insn_info)
> {
> /* Sets to a subreg of a multiword register are partial.
> Sets to a non-subreg of a multiword register are not. */
> if (GET_CODE (reg) == SUBREG)
> ref_flags |= DF_REF_PARTIAL;
> ref_flags |= DF_REF_MW_HARDREG;
>
> Corresponding use:
>
> None. However, this case should be rare to non-existent on most
> targets, and the current
Re: [stage 1 patch] remove unreachable code in expand_expr_real_1 (PR 21433)
On Fri, Feb 12, 2021 at 1:35 AM Martin Sebor via Gcc-patches
wrote:
>
> While trawling through old bugs I came across one from 2005: PR 21433
> - The COMPONENT_REF case of expand_expr_real_1 is probably wrong.
>
> The report looks correct in that argument 0 in COMPONENT_REF cannot
> be a CONSTRUCTOR. In my tests it's only been one of the following
> codes:
>
>array_ref
>component_ref
>mem_ref
>parm_decl
>result_decl
>var_decl
>
> The attached patch removes the CONSTRUCTOR code and replaces it with
> an assert verifying it doesn't come up there. Besides testing on
> x86_64-linux, the change is supported by comments in code and also
> in the internals manual (although that looks incorrect and should
> be changed to avoid suggesting the first operand is a decl).
Note the CTOR operand is valid GENERIC and likely came up before
we introduced GIMPLE. GIMPLE simply feeds more restrictive GENERIC
to the RTL expansion routines nowadays (so the patch is OK
eventually), but please
avoid altering GENERIC or tree.def documentation which documents
_GENERIC_.
The restricted boundary of GIMPLE -> RTL expansion is not documented
and in theory we might even run into your assert when processing
global initializers (in case the CTOR ends up TREE_CONSTANT).
> tree.def:
>
> /* Value is structure or union component.
> Operand 0 is the structure or union (an expression).
> Operand 1 is the field (a node of type FIELD_DECL).
> Operand 2, if present, is the value of DECL_FIELD_OFFSET, measured
> in units of DECL_OFFSET_ALIGN / BITS_PER_UNIT. */
> DEFTREECODE (COMPONENT_REF, "component_ref", tcc_reference, 3)
>
> generic.texi:
>
> @item COMPONENT_REF
> These nodes represent non-static data member accesses. The first
> operand is the object (rather than a pointer to it); the second operand
> is the @code{FIELD_DECL} for the data member. The third operand represents
> the byte offset of the field, but should not be used directly; call
> @code{component_ref_field_offset} instead.
Re: [PATCH] adjust "partly out of bounds" warning (PR 98503)
On Thu, Feb 11, 2021 at 11:26 PM Martin Sebor wrote: > > On 2/11/21 1:09 AM, Richard Biener wrote: > > On Wed, Feb 10, 2021 at 7:03 PM Martin Sebor wrote: > >> > >> On 2/10/21 3:39 AM, Richard Biener wrote: > >>> On Tue, Feb 9, 2021 at 4:37 PM Martin Sebor wrote: > > On 2/9/21 12:41 AM, Richard Biener wrote: > > On Tue, Feb 9, 2021 at 1:04 AM Martin Sebor via Gcc-patches > > wrote: > >> > >> On 2/8/21 12:09 PM, Jeff Law wrote: > >>> > >>> > >>> On 2/3/21 3:45 PM, Martin Sebor wrote: > On 2/3/21 2:57 PM, Jeff Law wrote: > > > > > > On 1/28/21 4:03 PM, Martin Sebor wrote: > >> The GCC 11 -Warray-bounds enhancement to diagnose accesses whose > >> leading offset is in bounds but whose trailing offset is not has > >> been causing some confusion. When the warning is issued for > >> an access to an in-bounds member via a pointer to a struct that's > >> larger than the pointed-to object, phrasing this strictly invalid > >> access in terms of array subscripts can be misleading, especially > >> when the source code doesn't involve any arrays or indexing. > >> > >> Since the problem boils down to an aliasing violation much more > >> so than an actual out-of-bounds access, the attached patch adjusts > >> the code to issue a -Wstrict-aliasing warning in these cases > >> instead > >> of -Warray-bounds. In addition, since the aliasing assumptions in > >> GCC can be disabled by -fno-strict-aliasing, the patch also makes > >> these instances of the warning conditional on -fstrict-aliasing > >> being in effect. > >> > >> Martin > >> > >> gcc-98503.diff > >> > >> PR middle-end/98503 -Warray-bounds when -Wstrict-aliasing would be > >> more appropriate > >> > >> gcc/ChangeLog: > >> > >> PR middle-end/98503 > >> * gimple-array-bounds.cc > >> (array_bounds_checker::check_mem_ref): > >> Issue -Wstrict-aliasing for a subset of violations. > >> (array_bounds_checker::check_array_bounds): Set new > >> member. > >> * gimple-array-bounds.h > >> (array_bounds_checker::cref_of_mref): New > >> data member. > >> > >> gcc/testsuite/ChangeLog: > >> > >> PR middle-end/98503 > >> * g++.dg/warn/Warray-bounds-10.C: Adjust text of expected > >> warnings. > >> * g++.dg/warn/Warray-bounds-11.C: Same. > >> * g++.dg/warn/Warray-bounds-12.C: Same. > >> * g++.dg/warn/Warray-bounds-13.C: Same. > >> * gcc.dg/Warray-bounds-63.c: Avoid -Wstrict-aliasing. > >> Adjust text > >> of expected warnings. > >> * gcc.dg/Warray-bounds-66.c: Adjust text of expected > >> warnings. > >> * gcc.dg/Wstrict-aliasing-2.c: New test. > >> * gcc.dg/Wstrict-aliasing-3.c: New test. > > What I don't like here is the strict-aliasing warnings inside the > > file > > and analysis phase for array bounds checking. > > > > ISTM that catching this at cast time would be better. So perhaps in > > build_c_cast and the C++ equivalent? > > > > It would mean we're warning at the cast site rather than the > > dereference, but that may ultimately be better for the warning > > anyway. > > I'm not sure. > > I had actually experimented with a this (in the middle end, and only > for accesses) but even that caused so many warnings that I abandoned > it, at least for now. -Warray-bounds has the benefit of flow > analysis > (and dead code elimination). In the front end it would have neither > and be both excessively noisy and ineffective at the same time. For > example: > >>> I think we agree that this really is an aliasing issue and I would go > >>> further and claim that it has nothing to do with array bounds > >>> checking. > >>> Therefore warning for it within gimple-array-bounds just seems wrong. > >>> > >>> I realize that you like having DCE run and the ability to look at > >>> offsets and such, but it really feels like the wrong place to do this. > >>> Aliasing issues are also more of a front-end thing since the language > >>> defines what is and what is not valid aliasing -- one might even argue > >>> that any aliasing warning needs to be identified by the front-ends > >>> exclusively (though possibly pruned away later). > >> > >> The aliasing restrictions are on accesses, which are [defined in > >> C as] execution-time actions on objects. Analyzing
