* [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]
@ 2023-09-18 16:12 Patrick Palka
2023-09-18 16:12 ` [PATCH v2 2/2] c++: convert_to_void and volatile references Patrick Palka
2023-09-18 16:19 ` [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
0 siblings, 2 replies; 8+ messages in thread
From: Patrick Palka @ 2023-09-18 16:12 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, Patrick Palka
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?
-- >8 --
Here convert_to_void always completes the type of an indirection or
id-expression, but according to [expr.context] an lvalue-to-rvalue
conversion is applied to a discarded-value expression only if "the
expression is a glvalue of volatile-qualified type". This patch
restricts convert_to_void's type completion to match.
PR c++/111419
gcc/cp/ChangeLog:
* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
complete_type if the type is volatile.
<case VAR_DECL>: Only call complete_type if the type is volatile.
Likewise.
gcc/testsuite/ChangeLog:
* g++.dg/cpp2a/concepts-requires36.C: New test.
* g++.dg/expr/discarded1.C: New test.
* g++.dg/expr/discarded1a.C: New test.
---
gcc/cp/cvt.cc | 10 ++++++----
.../g++.dg/cpp2a/concepts-requires36.C | 16 ++++++++++++++++
gcc/testsuite/g++.dg/expr/discarded1.C | 15 +++++++++++++++
gcc/testsuite/g++.dg/expr/discarded1a.C | 17 +++++++++++++++++
4 files changed, 54 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
create mode 100644 gcc/testsuite/g++.dg/expr/discarded1.C
create mode 100644 gcc/testsuite/g++.dg/expr/discarded1a.C
diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index c6b52f07050..4424670356c 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1250,9 +1250,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
case INDIRECT_REF:
{
tree type = TREE_TYPE (expr);
- int is_reference = TYPE_REF_P (TREE_TYPE (TREE_OPERAND (expr, 0)));
int is_volatile = TYPE_VOLATILE (type);
- int is_complete = COMPLETE_TYPE_P (complete_type (type));
+ if (is_volatile)
+ complete_type (type);
+ int is_complete = COMPLETE_TYPE_P (type);
/* Can't load the value if we don't know the type. */
if (is_volatile && !is_complete)
@@ -1412,9 +1413,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
{
/* External variables might be incomplete. */
tree type = TREE_TYPE (expr);
- int is_complete = COMPLETE_TYPE_P (complete_type (type));
- if (TYPE_VOLATILE (type) && !is_complete && (complain & tf_warning))
+ if (TYPE_VOLATILE (type)
+ && !COMPLETE_TYPE_P (complete_type (type))
+ && (complain & tf_warning))
switch (implicit)
{
case ICV_CAST:
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
new file mode 100644
index 00000000000..8d3a4fcd2aa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
@@ -0,0 +1,16 @@
+// PR c++/111419
+// { dg-do compile { target c++20 } }
+
+template<class F>
+concept invocable = requires(F& f) { f(); };
+
+template<class F>
+concept deref_invocable = requires(F& f) { *f(); };
+
+struct Incomplete;
+
+template<class T>
+struct Holder { T t; };
+
+static_assert(invocable<Holder<Incomplete>& ()>);
+static_assert(deref_invocable<Holder<Incomplete>* ()>);
diff --git a/gcc/testsuite/g++.dg/expr/discarded1.C b/gcc/testsuite/g++.dg/expr/discarded1.C
new file mode 100644
index 00000000000..c0c22e9e95b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/discarded1.C
@@ -0,0 +1,15 @@
+// PR c++/111419
+
+struct Incomplete;
+
+template<class T> struct Holder { T t; }; // { dg-bogus "incomplete" }
+
+extern Holder<Incomplete> a;
+extern Holder<Incomplete>& b;
+extern Holder<Incomplete>* c;
+
+int main() {
+ a;
+ b;
+ *c;
+}
diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
new file mode 100644
index 00000000000..1c4dff4553e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
@@ -0,0 +1,17 @@
+// A version of discarded1.C using volatile types.
+// PR c++/111419
+
+struct Incomplete;
+
+template<class T, int> struct Holder { T t; }; // { dg-error "incomplete" }
+
+extern volatile Holder<Incomplete, 0> a;
+extern volatile Holder<Incomplete, 1>& b;
+extern volatile Holder<Incomplete, 2>* c;
+
+int main() {
+ a; // { dg-message "required from here" }
+ b; // { dg-message "required from here" }
+ // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
+ *c; // { dg-message "required from here" }
+}
--
2.42.0.216.gbda494f404
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] c++: convert_to_void and volatile references
2023-09-18 16:12 [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
@ 2023-09-18 16:12 ` Patrick Palka
2023-09-18 16:31 ` Patrick Palka
2023-09-19 1:44 ` Jason Merrill
2023-09-18 16:19 ` [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
1 sibling, 2 replies; 8+ messages in thread
From: Patrick Palka @ 2023-09-18 16:12 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, Patrick Palka
Jason pointed out that even implicit loads of volatile references need
to undergo lvalue-to-rvalue conversion, but we currently emit a warning
in this case and discard the load. This patch changes this behavior so
that we don't issue a warning, and preserve the load.
gcc/cp/ChangeLog:
* cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
for an implicit load of a volatile reference. Simplify as if
is_reference is false. Check REFERENCE_REF_P in the test
guarding the -Wunused-value diagnostic.
gcc/testsuite/ChangeLog:
* g++.dg/expr/discarded1a.C: No longer expect warning for
implicit load of a volatile reference.
* g++.old-deja/g++.bugs/900428_01.C: Likewise.
* g++.dg/expr/volatile2.C: New test.
---
gcc/cp/cvt.cc | 56 ++-----------------
gcc/testsuite/g++.dg/expr/discarded1a.C | 1 -
gcc/testsuite/g++.dg/expr/volatile2.C | 12 ++++
.../g++.old-deja/g++.bugs/900428_01.C | 26 ++++-----
4 files changed, 30 insertions(+), 65 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index 4424670356c..1cb6c1222c2 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
{
tree type = TREE_TYPE (expr);
int is_volatile = TYPE_VOLATILE (type);
- if (is_volatile)
- complete_type (type);
- int is_complete = COMPLETE_TYPE_P (type);
/* Can't load the value if we don't know the type. */
- if (is_volatile && !is_complete)
+ if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
{
if (complain & tf_warning)
switch (implicit)
@@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
gcc_unreachable ();
}
}
- /* Don't load the value if this is an implicit dereference, or if
- the type needs to be handled by ctors/dtors. */
- else if (is_volatile && is_reference)
- {
- if (complain & tf_warning)
- switch (implicit)
- {
- case ICV_CAST:
- warning_at (loc, 0, "conversion to void will not access "
- "object of type %qT", type);
- break;
- case ICV_SECOND_OF_COND:
- warning_at (loc, 0, "implicit dereference will not access "
- "object of type %qT in second operand of "
- "conditional expression", type);
- break;
- case ICV_THIRD_OF_COND:
- warning_at (loc, 0, "implicit dereference will not access "
- "object of type %qT in third operand of "
- "conditional expression", type);
- break;
- case ICV_RIGHT_OF_COMMA:
- warning_at (loc, 0, "implicit dereference will not access "
- "object of type %qT in right operand of "
- "comma operator", type);
- break;
- case ICV_LEFT_OF_COMMA:
- warning_at (loc, 0, "implicit dereference will not access "
- "object of type %qT in left operand of comma "
- "operator", type);
- break;
- case ICV_STATEMENT:
- warning_at (loc, 0, "implicit dereference will not access "
- "object of type %qT in statement", type);
- break;
- case ICV_THIRD_IN_FOR:
- warning_at (loc, 0, "implicit dereference will not access "
- "object of type %qT in for increment expression",
- type);
- break;
- default:
- gcc_unreachable ();
- }
- }
+ /* Don't load the value if the type needs to be handled by cdtors. */
else if (is_volatile && TREE_ADDRESSABLE (type))
{
if (complain & tf_warning)
@@ -1386,7 +1340,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
gcc_unreachable ();
}
}
- if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE (type))
+ if (!is_volatile || !COMPLETE_TYPE_P (type))
{
/* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
operation is stripped off. Note that we don't warn about
@@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
if (warn_unused_value
&& implicit != ICV_CAST
&& (complain & tf_warning)
- && !warning_suppressed_p (expr, OPT_Wunused_value)
- && !is_reference)
+ && !warning_suppressed_p (expr, OPT_Wunused_value)
+ && !REFERENCE_REF_P (expr))
warning_at (loc, OPT_Wunused_value, "value computed is not used");
expr = TREE_OPERAND (expr, 0);
if (TREE_CODE (expr) == CALL_EXPR
diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
index 1c4dff4553e..4754f32f716 100644
--- a/gcc/testsuite/g++.dg/expr/discarded1a.C
+++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
@@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
int main() {
a; // { dg-message "required from here" }
b; // { dg-message "required from here" }
- // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
*c; // { dg-message "required from here" }
}
diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C b/gcc/testsuite/g++.dg/expr/volatile2.C
new file mode 100644
index 00000000000..62e609aee2f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/volatile2.C
@@ -0,0 +1,12 @@
+// Verify we preserve implicit loads of volatile references in a
+// discarded-value expression.
+// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized" }
+// { dg-final { scan-tree-dump {\*a} "original" } }
+// { dg-final { scan-tree-dump {\*a} "optimized" } }
+
+struct A { int m; };
+extern volatile A& a;
+
+int main() {
+ a;
+}
diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
index a806ef0706e..2b82c64ff73 100644
--- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
+++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
@@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r, volatile int &vr)
(void)(i ? r : j); // ok, no warning
(void)((void)1, r); // ok, no warning
- vr; // { dg-warning "" } reference not accessed
- (void)vr; // { dg-warning "" } reference not accessed
- (void)(i ? vj : vr); // { dg-warning "" } reference not accessed
- (void)(i ? vr : vj); // { dg-warning "" } reference not accessed
- (void)((void)1, vr); // { dg-warning "" } reference not accessed
+ vr; // ok, no warning
+ (void)vr; // ok, no warning
+ (void)(i ? vj : vr); // ok, no warning
+ (void)(i ? vr : vj); // ok, no warning
+ (void)((void)1, vr); // ok, no warning
*ip_fn (); // ok, no warning
*vip_fn (); // ok, no warning
ir_fn (); // ok, no warning
- vir_fn (); // { dg-warning "" } reference not accessed
+ vir_fn (); // ok, no warning
}
struct S;
@@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r, volatile T &vr)
(void)(i ? r : j); // ok, no warning
(void)((void)1, r); // ok, no warning
- vr; // { dg-warning "" } reference not accessed
- (void)vr; // { dg-warning "" } reference not accessed
- (void)(i ? vj : vr); // { dg-warning "" } reference not accessed
- (void)(i ? vr : vj); // { dg-warning "" } reference not accessed
- (void)((void)1, vr); // { dg-warning "" } reference not accessed
+ vr; // ok, no warning
+ (void)vr; // ok, no warning
+ (void)(i ? vj : vr); // ok, no warning
+ (void)(i ? vr : vj); // ok, no warning
+ (void)((void)1, vr); // ok, no warning
*tp_fn (); // ok, no warning
*vtp_fn (); // ok, no warning
tr_fn (); // ok, no warning
- vtr_fn (); // ok, no warning{ dg-warning "" } reference not accessed
+ vtr_fn (); // ok, no warning
}
void extern_test ()
@@ -160,5 +160,5 @@ void extern_test ()
esr; // ok, no warning
vesr; // { dg-warning "" } incomplete not accessed
etr; // ok, no warning
- vetr; // { dg-warning "" } reference not accessed
+ vetr; // ok, no warning
}
--
2.42.0.216.gbda494f404
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]
2023-09-18 16:12 [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
2023-09-18 16:12 ` [PATCH v2 2/2] c++: convert_to_void and volatile references Patrick Palka
@ 2023-09-18 16:19 ` Patrick Palka
2023-09-19 1:45 ` Jason Merrill
1 sibling, 1 reply; 8+ messages in thread
From: Patrick Palka @ 2023-09-18 16:19 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, jason
On Mon, 18 Sep 2023, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
>
> -- >8 --
>
> Here convert_to_void always completes the type of an indirection or
> id-expression, but according to [expr.context] an lvalue-to-rvalue
> conversion is applied to a discarded-value expression only if "the
> expression is a glvalue of volatile-qualified type". This patch
> restricts convert_to_void's type completion to match.
>
> PR c++/111419
>
> gcc/cp/ChangeLog:
>
> * cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> complete_type if the type is volatile.
> <case VAR_DECL>: Only call complete_type if the type is volatile.
> Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp2a/concepts-requires36.C: New test.
> * g++.dg/expr/discarded1.C: New test.
> * g++.dg/expr/discarded1a.C: New test.
> ---
> gcc/cp/cvt.cc | 10 ++++++----
> .../g++.dg/cpp2a/concepts-requires36.C | 16 ++++++++++++++++
> gcc/testsuite/g++.dg/expr/discarded1.C | 15 +++++++++++++++
> gcc/testsuite/g++.dg/expr/discarded1a.C | 17 +++++++++++++++++
> 4 files changed, 54 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
> create mode 100644 gcc/testsuite/g++.dg/expr/discarded1.C
> create mode 100644 gcc/testsuite/g++.dg/expr/discarded1a.C
>
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index c6b52f07050..4424670356c 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1250,9 +1250,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> case INDIRECT_REF:
> {
> tree type = TREE_TYPE (expr);
> - int is_reference = TYPE_REF_P (TREE_TYPE (TREE_OPERAND (expr, 0)));
Oops, this removal of is_reference should happen in the follow-up patch
instead, consider that fixed.
> int is_volatile = TYPE_VOLATILE (type);
> - int is_complete = COMPLETE_TYPE_P (complete_type (type));
> + if (is_volatile)
> + complete_type (type);
> + int is_complete = COMPLETE_TYPE_P (type);
>
> /* Can't load the value if we don't know the type. */
> if (is_volatile && !is_complete)
> @@ -1412,9 +1413,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> {
> /* External variables might be incomplete. */
> tree type = TREE_TYPE (expr);
> - int is_complete = COMPLETE_TYPE_P (complete_type (type));
>
> - if (TYPE_VOLATILE (type) && !is_complete && (complain & tf_warning))
> + if (TYPE_VOLATILE (type)
> + && !COMPLETE_TYPE_P (complete_type (type))
> + && (complain & tf_warning))
> switch (implicit)
> {
> case ICV_CAST:
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
> new file mode 100644
> index 00000000000..8d3a4fcd2aa
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
> @@ -0,0 +1,16 @@
> +// PR c++/111419
> +// { dg-do compile { target c++20 } }
> +
> +template<class F>
> +concept invocable = requires(F& f) { f(); };
> +
> +template<class F>
> +concept deref_invocable = requires(F& f) { *f(); };
> +
> +struct Incomplete;
> +
> +template<class T>
> +struct Holder { T t; };
> +
> +static_assert(invocable<Holder<Incomplete>& ()>);
> +static_assert(deref_invocable<Holder<Incomplete>* ()>);
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1.C b/gcc/testsuite/g++.dg/expr/discarded1.C
> new file mode 100644
> index 00000000000..c0c22e9e95b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/discarded1.C
> @@ -0,0 +1,15 @@
> +// PR c++/111419
> +
> +struct Incomplete;
> +
> +template<class T> struct Holder { T t; }; // { dg-bogus "incomplete" }
> +
> +extern Holder<Incomplete> a;
> +extern Holder<Incomplete>& b;
> +extern Holder<Incomplete>* c;
> +
> +int main() {
> + a;
> + b;
> + *c;
> +}
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> new file mode 100644
> index 00000000000..1c4dff4553e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -0,0 +1,17 @@
> +// A version of discarded1.C using volatile types.
> +// PR c++/111419
> +
> +struct Incomplete;
> +
> +template<class T, int> struct Holder { T t; }; // { dg-error "incomplete" }
> +
> +extern volatile Holder<Incomplete, 0> a;
> +extern volatile Holder<Incomplete, 1>& b;
> +extern volatile Holder<Incomplete, 2>* c;
> +
> +int main() {
> + a; // { dg-message "required from here" }
> + b; // { dg-message "required from here" }
> + // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
> + *c; // { dg-message "required from here" }
> +}
> --
> 2.42.0.216.gbda494f404
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] c++: convert_to_void and volatile references
2023-09-18 16:12 ` [PATCH v2 2/2] c++: convert_to_void and volatile references Patrick Palka
@ 2023-09-18 16:31 ` Patrick Palka
2023-09-19 1:44 ` Jason Merrill
1 sibling, 0 replies; 8+ messages in thread
From: Patrick Palka @ 2023-09-18 16:31 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, jason
On Mon, 18 Sep 2023, Patrick Palka wrote:
> Jason pointed out that even implicit loads of volatile references need
> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> in this case and discard the load. This patch changes this behavior so
> that we don't issue a warning, and preserve the load.
>
> gcc/cp/ChangeLog:
>
> * cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
> for an implicit load of a volatile reference. Simplify as if
> is_reference is false. Check REFERENCE_REF_P in the test
> guarding the -Wunused-value diagnostic.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/expr/discarded1a.C: No longer expect warning for
> implicit load of a volatile reference.
> * g++.old-deja/g++.bugs/900428_01.C: Likewise.
> * g++.dg/expr/volatile2.C: New test.
> ---
> gcc/cp/cvt.cc | 56 ++-----------------
> gcc/testsuite/g++.dg/expr/discarded1a.C | 1 -
> gcc/testsuite/g++.dg/expr/volatile2.C | 12 ++++
> .../g++.old-deja/g++.bugs/900428_01.C | 26 ++++-----
> 4 files changed, 30 insertions(+), 65 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
>
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index 4424670356c..1cb6c1222c2 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> {
> tree type = TREE_TYPE (expr);
> int is_volatile = TYPE_VOLATILE (type);
> - if (is_volatile)
> - complete_type (type);
> - int is_complete = COMPLETE_TYPE_P (type);
>
> /* Can't load the value if we don't know the type. */
> - if (is_volatile && !is_complete)
> + if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
> {
> if (complain & tf_warning)
> switch (implicit)
> @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> gcc_unreachable ();
> }
> }
> - /* Don't load the value if this is an implicit dereference, or if
> - the type needs to be handled by ctors/dtors. */
> - else if (is_volatile && is_reference)
> - {
> - if (complain & tf_warning)
> - switch (implicit)
> - {
> - case ICV_CAST:
> - warning_at (loc, 0, "conversion to void will not access "
> - "object of type %qT", type);
> - break;
> - case ICV_SECOND_OF_COND:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in second operand of "
> - "conditional expression", type);
> - break;
> - case ICV_THIRD_OF_COND:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in third operand of "
> - "conditional expression", type);
> - break;
> - case ICV_RIGHT_OF_COMMA:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in right operand of "
> - "comma operator", type);
> - break;
> - case ICV_LEFT_OF_COMMA:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in left operand of comma "
> - "operator", type);
> - break;
> - case ICV_STATEMENT:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in statement", type);
> - break;
> - case ICV_THIRD_IN_FOR:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in for increment expression",
> - type);
> - break;
> - default:
> - gcc_unreachable ();
> - }
> - }
> + /* Don't load the value if the type needs to be handled by cdtors. */
> else if (is_volatile && TREE_ADDRESSABLE (type))
> {
> if (complain & tf_warning)
> @@ -1386,7 +1340,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> gcc_unreachable ();
> }
> }
> - if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE (type))
> + if (!is_volatile || !COMPLETE_TYPE_P (type))
... and the TREE_ADDRESSABLE test should not have been removed, consider
that fixed. (The original patch doesn't have this mistake, I just
messed up splitting the patch into two.)
> {
> /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
> operation is stripped off. Note that we don't warn about
> @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> if (warn_unused_value
> && implicit != ICV_CAST
> && (complain & tf_warning)
> - && !warning_suppressed_p (expr, OPT_Wunused_value)
> - && !is_reference)
> + && !warning_suppressed_p (expr, OPT_Wunused_value)
> + && !REFERENCE_REF_P (expr))
> warning_at (loc, OPT_Wunused_value, "value computed is not used");
> expr = TREE_OPERAND (expr, 0);
> if (TREE_CODE (expr) == CALL_EXPR
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> index 1c4dff4553e..4754f32f716 100644
> --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
> int main() {
> a; // { dg-message "required from here" }
> b; // { dg-message "required from here" }
> - // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
> *c; // { dg-message "required from here" }
> }
> diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C b/gcc/testsuite/g++.dg/expr/volatile2.C
> new file mode 100644
> index 00000000000..62e609aee2f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> @@ -0,0 +1,12 @@
> +// Verify we preserve implicit loads of volatile references in a
> +// discarded-value expression.
> +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized" }
> +// { dg-final { scan-tree-dump {\*a} "original" } }
> +// { dg-final { scan-tree-dump {\*a} "optimized" } }
> +
> +struct A { int m; };
> +extern volatile A& a;
> +
> +int main() {
> + a;
> +}
> diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> index a806ef0706e..2b82c64ff73 100644
> --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r, volatile int &vr)
> (void)(i ? r : j); // ok, no warning
> (void)((void)1, r); // ok, no warning
>
> - vr; // { dg-warning "" } reference not accessed
> - (void)vr; // { dg-warning "" } reference not accessed
> - (void)(i ? vj : vr); // { dg-warning "" } reference not accessed
> - (void)(i ? vr : vj); // { dg-warning "" } reference not accessed
> - (void)((void)1, vr); // { dg-warning "" } reference not accessed
> + vr; // ok, no warning
> + (void)vr; // ok, no warning
> + (void)(i ? vj : vr); // ok, no warning
> + (void)(i ? vr : vj); // ok, no warning
> + (void)((void)1, vr); // ok, no warning
>
> *ip_fn (); // ok, no warning
> *vip_fn (); // ok, no warning
> ir_fn (); // ok, no warning
> - vir_fn (); // { dg-warning "" } reference not accessed
> + vir_fn (); // ok, no warning
> }
>
> struct S;
> @@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r, volatile T &vr)
> (void)(i ? r : j); // ok, no warning
> (void)((void)1, r); // ok, no warning
>
> - vr; // { dg-warning "" } reference not accessed
> - (void)vr; // { dg-warning "" } reference not accessed
> - (void)(i ? vj : vr); // { dg-warning "" } reference not accessed
> - (void)(i ? vr : vj); // { dg-warning "" } reference not accessed
> - (void)((void)1, vr); // { dg-warning "" } reference not accessed
> + vr; // ok, no warning
> + (void)vr; // ok, no warning
> + (void)(i ? vj : vr); // ok, no warning
> + (void)(i ? vr : vj); // ok, no warning
> + (void)((void)1, vr); // ok, no warning
>
> *tp_fn (); // ok, no warning
> *vtp_fn (); // ok, no warning
> tr_fn (); // ok, no warning
> - vtr_fn (); // ok, no warning{ dg-warning "" } reference not accessed
> + vtr_fn (); // ok, no warning
> }
>
> void extern_test ()
> @@ -160,5 +160,5 @@ void extern_test ()
> esr; // ok, no warning
> vesr; // { dg-warning "" } incomplete not accessed
> etr; // ok, no warning
> - vetr; // { dg-warning "" } reference not accessed
> + vetr; // ok, no warning
> }
> --
> 2.42.0.216.gbda494f404
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] c++: convert_to_void and volatile references
2023-09-18 16:12 ` [PATCH v2 2/2] c++: convert_to_void and volatile references Patrick Palka
2023-09-18 16:31 ` Patrick Palka
@ 2023-09-19 1:44 ` Jason Merrill
2023-09-19 17:53 ` Patrick Palka
1 sibling, 1 reply; 8+ messages in thread
From: Jason Merrill @ 2023-09-19 1:44 UTC (permalink / raw)
To: Patrick Palka, gcc-patches
On 9/18/23 12:12, Patrick Palka wrote:
> Jason pointed out that even implicit loads of volatile references need
> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> in this case and discard the load. This patch changes this behavior so
> that we don't issue a warning, and preserve the load.
>
> gcc/cp/ChangeLog:
>
> * cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
> for an implicit load of a volatile reference. Simplify as if
> is_reference is false. Check REFERENCE_REF_P in the test
> guarding the -Wunused-value diagnostic.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/expr/discarded1a.C: No longer expect warning for
> implicit load of a volatile reference.
> * g++.old-deja/g++.bugs/900428_01.C: Likewise.
> * g++.dg/expr/volatile2.C: New test.
> ---
> gcc/cp/cvt.cc | 56 ++-----------------
> gcc/testsuite/g++.dg/expr/discarded1a.C | 1 -
> gcc/testsuite/g++.dg/expr/volatile2.C | 12 ++++
> .../g++.old-deja/g++.bugs/900428_01.C | 26 ++++-----
> 4 files changed, 30 insertions(+), 65 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
>
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index 4424670356c..1cb6c1222c2 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> {
> tree type = TREE_TYPE (expr);
> int is_volatile = TYPE_VOLATILE (type);
> - if (is_volatile)
> - complete_type (type);
> - int is_complete = COMPLETE_TYPE_P (type);
>
> /* Can't load the value if we don't know the type. */
> - if (is_volatile && !is_complete)
> + if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
> {
> if (complain & tf_warning)
> switch (implicit)
> @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> gcc_unreachable ();
> }
> }
> - /* Don't load the value if this is an implicit dereference, or if
> - the type needs to be handled by ctors/dtors. */
> - else if (is_volatile && is_reference)
> - {
> - if (complain & tf_warning)
> - switch (implicit)
> - {
> - case ICV_CAST:
> - warning_at (loc, 0, "conversion to void will not access "
> - "object of type %qT", type);
> - break;
> - case ICV_SECOND_OF_COND:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in second operand of "
> - "conditional expression", type);
> - break;
> - case ICV_THIRD_OF_COND:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in third operand of "
> - "conditional expression", type);
> - break;
> - case ICV_RIGHT_OF_COMMA:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in right operand of "
> - "comma operator", type);
> - break;
> - case ICV_LEFT_OF_COMMA:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in left operand of comma "
> - "operator", type);
> - break;
> - case ICV_STATEMENT:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in statement", type);
> - break;
> - case ICV_THIRD_IN_FOR:
> - warning_at (loc, 0, "implicit dereference will not access "
> - "object of type %qT in for increment expression",
> - type);
> - break;
> - default:
> - gcc_unreachable ();
> - }
> - }
> + /* Don't load the value if the type needs to be handled by cdtors. */
> else if (is_volatile && TREE_ADDRESSABLE (type))
> {
> if (complain & tf_warning)
> @@ -1386,7 +1340,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> gcc_unreachable ();
> }
> }
> - if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE (type))
> + if (!is_volatile || !COMPLETE_TYPE_P (type))
> {
> /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
> operation is stripped off. Note that we don't warn about
> @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
> if (warn_unused_value
> && implicit != ICV_CAST
> && (complain & tf_warning)
> - && !warning_suppressed_p (expr, OPT_Wunused_value)
> - && !is_reference)
> + && !warning_suppressed_p (expr, OPT_Wunused_value)
> + && !REFERENCE_REF_P (expr))
> warning_at (loc, OPT_Wunused_value, "value computed is not used");
> expr = TREE_OPERAND (expr, 0);
> if (TREE_CODE (expr) == CALL_EXPR
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> index 1c4dff4553e..4754f32f716 100644
> --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
> int main() {
> a; // { dg-message "required from here" }
> b; // { dg-message "required from here" }
> - // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
> *c; // { dg-message "required from here" }
> }
> diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C b/gcc/testsuite/g++.dg/expr/volatile2.C
> new file mode 100644
> index 00000000000..62e609aee2f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> @@ -0,0 +1,12 @@
> +// Verify we preserve implicit loads of volatile references in a
> +// discarded-value expression.
> +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized" }
> +// { dg-final { scan-tree-dump {\*a} "original" } }
> +// { dg-final { scan-tree-dump {\*a} "optimized" } }
> +
> +struct A { int m; };
> +extern volatile A& a;
> +
> +int main() {
> + a;
> +}
> diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> index a806ef0706e..2b82c64ff73 100644
> --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r, volatile int &vr)
> (void)(i ? r : j); // ok, no warning
> (void)((void)1, r); // ok, no warning
>
> - vr; // { dg-warning "" } reference not accessed
> - (void)vr; // { dg-warning "" } reference not accessed
> - (void)(i ? vj : vr); // { dg-warning "" } reference not accessed
> - (void)(i ? vr : vj); // { dg-warning "" } reference not accessed
> - (void)((void)1, vr); // { dg-warning "" } reference not accessed
> + vr; // ok, no warning
> + (void)vr; // ok, no warning
> + (void)(i ? vj : vr); // ok, no warning
> + (void)(i ? vr : vj); // ok, no warning
> + (void)((void)1, vr); // ok, no warning
>
> *ip_fn (); // ok, no warning
> *vip_fn (); // ok, no warning
> ir_fn (); // ok, no warning
> - vir_fn (); // { dg-warning "" } reference not accessed
> + vir_fn (); // ok, no warning
This should still warn (and not load), a function call is not one of the
forms in https://eel.is/c++draft/expr#context-2. We still need the
warnings you're removing for expression forms that aren't on that list.
> }
>
> struct S;
> @@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r, volatile T &vr)
> (void)(i ? r : j); // ok, no warning
> (void)((void)1, r); // ok, no warning
>
> - vr; // { dg-warning "" } reference not accessed
> - (void)vr; // { dg-warning "" } reference not accessed
> - (void)(i ? vj : vr); // { dg-warning "" } reference not accessed
> - (void)(i ? vr : vj); // { dg-warning "" } reference not accessed
> - (void)((void)1, vr); // { dg-warning "" } reference not accessed
> + vr; // ok, no warning
> + (void)vr; // ok, no warning
> + (void)(i ? vj : vr); // ok, no warning
> + (void)(i ? vr : vj); // ok, no warning
> + (void)((void)1, vr); // ok, no warning
>
> *tp_fn (); // ok, no warning
> *vtp_fn (); // ok, no warning
> tr_fn (); // ok, no warning
> - vtr_fn (); // ok, no warning{ dg-warning "" } reference not accessed
> + vtr_fn (); // ok, no warning
Likewise.
> }
>
> void extern_test ()
> @@ -160,5 +160,5 @@ void extern_test ()
> esr; // ok, no warning
> vesr; // { dg-warning "" } incomplete not accessed
> etr; // ok, no warning
> - vetr; // { dg-warning "" } reference not accessed
> + vetr; // ok, no warning
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]
2023-09-18 16:19 ` [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
@ 2023-09-19 1:45 ` Jason Merrill
0 siblings, 0 replies; 8+ messages in thread
From: Jason Merrill @ 2023-09-19 1:45 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches
On 9/18/23 12:19, Patrick Palka wrote:
> On Mon, 18 Sep 2023, Patrick Palka wrote:
>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>> trunk?
>>
>> -- >8 --
>>
>> Here convert_to_void always completes the type of an indirection or
>> id-expression, but according to [expr.context] an lvalue-to-rvalue
>> conversion is applied to a discarded-value expression only if "the
>> expression is a glvalue of volatile-qualified type". This patch
>> restricts convert_to_void's type completion to match.
>>
>> PR c++/111419
>>
>> gcc/cp/ChangeLog:
>>
>> * cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
>> complete_type if the type is volatile.
>> <case VAR_DECL>: Only call complete_type if the type is volatile.
>> Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.dg/cpp2a/concepts-requires36.C: New test.
>> * g++.dg/expr/discarded1.C: New test.
>> * g++.dg/expr/discarded1a.C: New test.
>> ---
>> gcc/cp/cvt.cc | 10 ++++++----
>> .../g++.dg/cpp2a/concepts-requires36.C | 16 ++++++++++++++++
>> gcc/testsuite/g++.dg/expr/discarded1.C | 15 +++++++++++++++
>> gcc/testsuite/g++.dg/expr/discarded1a.C | 17 +++++++++++++++++
>> 4 files changed, 54 insertions(+), 4 deletions(-)
>> create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>> create mode 100644 gcc/testsuite/g++.dg/expr/discarded1.C
>> create mode 100644 gcc/testsuite/g++.dg/expr/discarded1a.C
>>
>> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
>> index c6b52f07050..4424670356c 100644
>> --- a/gcc/cp/cvt.cc
>> +++ b/gcc/cp/cvt.cc
>> @@ -1250,9 +1250,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>> case INDIRECT_REF:
>> {
>> tree type = TREE_TYPE (expr);
>> - int is_reference = TYPE_REF_P (TREE_TYPE (TREE_OPERAND (expr, 0)));
>
> Oops, this removal of is_reference should happen in the follow-up patch
> instead, consider that fixed.
OK with that fixed.
>> int is_volatile = TYPE_VOLATILE (type);
>> - int is_complete = COMPLETE_TYPE_P (complete_type (type));
>> + if (is_volatile)
>> + complete_type (type);
>> + int is_complete = COMPLETE_TYPE_P (type);
>>
>> /* Can't load the value if we don't know the type. */
>> if (is_volatile && !is_complete)
>> @@ -1412,9 +1413,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>> {
>> /* External variables might be incomplete. */
>> tree type = TREE_TYPE (expr);
>> - int is_complete = COMPLETE_TYPE_P (complete_type (type));
>>
>> - if (TYPE_VOLATILE (type) && !is_complete && (complain & tf_warning))
>> + if (TYPE_VOLATILE (type)
>> + && !COMPLETE_TYPE_P (complete_type (type))
>> + && (complain & tf_warning))
>> switch (implicit)
>> {
>> case ICV_CAST:
>> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>> new file mode 100644
>> index 00000000000..8d3a4fcd2aa
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>> @@ -0,0 +1,16 @@
>> +// PR c++/111419
>> +// { dg-do compile { target c++20 } }
>> +
>> +template<class F>
>> +concept invocable = requires(F& f) { f(); };
>> +
>> +template<class F>
>> +concept deref_invocable = requires(F& f) { *f(); };
>> +
>> +struct Incomplete;
>> +
>> +template<class T>
>> +struct Holder { T t; };
>> +
>> +static_assert(invocable<Holder<Incomplete>& ()>);
>> +static_assert(deref_invocable<Holder<Incomplete>* ()>);
>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1.C b/gcc/testsuite/g++.dg/expr/discarded1.C
>> new file mode 100644
>> index 00000000000..c0c22e9e95b
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/expr/discarded1.C
>> @@ -0,0 +1,15 @@
>> +// PR c++/111419
>> +
>> +struct Incomplete;
>> +
>> +template<class T> struct Holder { T t; }; // { dg-bogus "incomplete" }
>> +
>> +extern Holder<Incomplete> a;
>> +extern Holder<Incomplete>& b;
>> +extern Holder<Incomplete>* c;
>> +
>> +int main() {
>> + a;
>> + b;
>> + *c;
>> +}
>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
>> new file mode 100644
>> index 00000000000..1c4dff4553e
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
>> @@ -0,0 +1,17 @@
>> +// A version of discarded1.C using volatile types.
>> +// PR c++/111419
>> +
>> +struct Incomplete;
>> +
>> +template<class T, int> struct Holder { T t; }; // { dg-error "incomplete" }
>> +
>> +extern volatile Holder<Incomplete, 0> a;
>> +extern volatile Holder<Incomplete, 1>& b;
>> +extern volatile Holder<Incomplete, 2>* c;
>> +
>> +int main() {
>> + a; // { dg-message "required from here" }
>> + b; // { dg-message "required from here" }
>> + // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
>> + *c; // { dg-message "required from here" }
>> +}
>> --
>> 2.42.0.216.gbda494f404
>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] c++: convert_to_void and volatile references
2023-09-19 1:44 ` Jason Merrill
@ 2023-09-19 17:53 ` Patrick Palka
2023-09-19 23:27 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Patrick Palka @ 2023-09-19 17:53 UTC (permalink / raw)
To: Jason Merrill; +Cc: Patrick Palka, gcc-patches
On Mon, 18 Sep 2023, Jason Merrill wrote:
> On 9/18/23 12:12, Patrick Palka wrote:
> > Jason pointed out that even implicit loads of volatile references need
> > to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> > in this case and discard the load. This patch changes this behavior so
> > that we don't issue a warning, and preserve the load.
> >
> > gcc/cp/ChangeLog:
> >
> > * cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
> > for an implicit load of a volatile reference. Simplify as if
> > is_reference is false. Check REFERENCE_REF_P in the test
> > guarding the -Wunused-value diagnostic.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/expr/discarded1a.C: No longer expect warning for
> > implicit load of a volatile reference.
> > * g++.old-deja/g++.bugs/900428_01.C: Likewise.
> > * g++.dg/expr/volatile2.C: New test.
> > ---
> > gcc/cp/cvt.cc | 56 ++-----------------
> > gcc/testsuite/g++.dg/expr/discarded1a.C | 1 -
> > gcc/testsuite/g++.dg/expr/volatile2.C | 12 ++++
> > .../g++.old-deja/g++.bugs/900428_01.C | 26 ++++-----
> > 4 files changed, 30 insertions(+), 65 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
> >
> > diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> > index 4424670356c..1cb6c1222c2 100644
> > --- a/gcc/cp/cvt.cc
> > +++ b/gcc/cp/cvt.cc
> > @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> > {
> > tree type = TREE_TYPE (expr);
> > int is_volatile = TYPE_VOLATILE (type);
> > - if (is_volatile)
> > - complete_type (type);
> > - int is_complete = COMPLETE_TYPE_P (type);
> > /* Can't load the value if we don't know the type. */
> > - if (is_volatile && !is_complete)
> > + if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
> > {
> > if (complain & tf_warning)
> > switch (implicit)
> > @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> > gcc_unreachable ();
> > }
> > }
> > - /* Don't load the value if this is an implicit dereference, or if
> > - the type needs to be handled by ctors/dtors. */
> > - else if (is_volatile && is_reference)
> > - {
> > - if (complain & tf_warning)
> > - switch (implicit)
> > - {
> > - case ICV_CAST:
> > - warning_at (loc, 0, "conversion to void will not access "
> > - "object of type %qT", type);
> > - break;
> > - case ICV_SECOND_OF_COND:
> > - warning_at (loc, 0, "implicit dereference will not access
> > "
> > - "object of type %qT in second operand of "
> > - "conditional expression", type);
> > - break;
> > - case ICV_THIRD_OF_COND:
> > - warning_at (loc, 0, "implicit dereference will not access
> > "
> > - "object of type %qT in third operand of "
> > - "conditional expression", type);
> > - break;
> > - case ICV_RIGHT_OF_COMMA:
> > - warning_at (loc, 0, "implicit dereference will not access
> > "
> > - "object of type %qT in right operand of "
> > - "comma operator", type);
> > - break;
> > - case ICV_LEFT_OF_COMMA:
> > - warning_at (loc, 0, "implicit dereference will not access
> > "
> > - "object of type %qT in left operand of comma "
> > - "operator", type);
> > - break;
> > - case ICV_STATEMENT:
> > - warning_at (loc, 0, "implicit dereference will not access
> > "
> > - "object of type %qT in statement", type);
> > - break;
> > - case ICV_THIRD_IN_FOR:
> > - warning_at (loc, 0, "implicit dereference will not access
> > "
> > - "object of type %qT in for increment
> > expression",
> > - type);
> > - break;
> > - default:
> > - gcc_unreachable ();
> > - }
> > - }
> > + /* Don't load the value if the type needs to be handled by cdtors. */
> > else if (is_volatile && TREE_ADDRESSABLE (type))
> > {
> > if (complain & tf_warning)
> > @@ -1386,7 +1340,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> > gcc_unreachable ();
> > }
> > }
> > - if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE
> > (type))
> > + if (!is_volatile || !COMPLETE_TYPE_P (type))
> > {
> > /* Emit a warning (if enabled) when the "effect-less"
> > INDIRECT_REF
> > operation is stripped off. Note that we don't warn about
> > @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> > if (warn_unused_value
> > && implicit != ICV_CAST
> > && (complain & tf_warning)
> > - && !warning_suppressed_p (expr, OPT_Wunused_value)
> > - && !is_reference)
> > + && !warning_suppressed_p (expr, OPT_Wunused_value)
> > + && !REFERENCE_REF_P (expr))
> > warning_at (loc, OPT_Wunused_value, "value computed is not
> > used");
> > expr = TREE_OPERAND (expr, 0);
> > if (TREE_CODE (expr) == CALL_EXPR
> > diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > index 1c4dff4553e..4754f32f716 100644
> > --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
> > int main() {
> > a; // { dg-message "required from here" }
> > b; // { dg-message "required from here" }
> > - // { dg-warning "implicit dereference will not access object" "" { target
> > *-*-* } .-1 }
> > *c; // { dg-message "required from here" }
> > }
> > diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C
> > b/gcc/testsuite/g++.dg/expr/volatile2.C
> > new file mode 100644
> > index 00000000000..62e609aee2f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> > @@ -0,0 +1,12 @@
> > +// Verify we preserve implicit loads of volatile references in a
> > +// discarded-value expression.
> > +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized"
> > }
> > +// { dg-final { scan-tree-dump {\*a} "original" } }
> > +// { dg-final { scan-tree-dump {\*a} "optimized" } }
> > +
> > +struct A { int m; };
> > +extern volatile A& a;
> > +
> > +int main() {
> > + a;
> > +}
> > diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > index a806ef0706e..2b82c64ff73 100644
> > --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r,
> > volatile int &vr)
> > (void)(i ? r : j); // ok, no warning
> > (void)((void)1, r); // ok, no warning
> > - vr; // { dg-warning "" } reference not
> > accessed
> > - (void)vr; // { dg-warning "" } reference not accessed
> > - (void)(i ? vj : vr); // { dg-warning "" } reference not
> > accessed
> > - (void)(i ? vr : vj); // { dg-warning "" } reference not
> > accessed
> > - (void)((void)1, vr); // { dg-warning "" } reference not accessed
> > + vr; // ok, no warning
> > + (void)vr; // ok, no warning
> > + (void)(i ? vj : vr); // ok, no warning
> > + (void)(i ? vr : vj); // ok, no warning
> > + (void)((void)1, vr); // ok, no warning
> > *ip_fn (); // ok, no warning
> > *vip_fn (); // ok, no warning
> > ir_fn (); // ok, no warning
> > - vir_fn (); // { dg-warning "" } reference not accessed
> > + vir_fn (); // ok, no warning
>
> This should still warn (and not load), a function call is not one of the forms
> in https://eel.is/c++draft/expr#context-2. We still need the warnings you're
> removing for expression forms that aren't on that list.
D'oh, will fix. It's surprising at first glance that *vip_fn() behaves
differently from vir_fn().
>
> > }
> > struct S;
> > @@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r,
> > volatile T &vr)
> > (void)(i ? r : j); // ok, no warning
> > (void)((void)1, r); // ok, no warning
> > - vr; // { dg-warning "" } reference not
> > accessed
> > - (void)vr; // { dg-warning "" } reference not accessed
> > - (void)(i ? vj : vr); // { dg-warning "" } reference not
> > accessed
> > - (void)(i ? vr : vj); // { dg-warning "" } reference not
> > accessed
> > - (void)((void)1, vr); // { dg-warning "" } reference not accessed
> > + vr; // ok, no warning
> > + (void)vr; // ok, no warning
> > + (void)(i ? vj : vr); // ok, no warning
> > + (void)(i ? vr : vj); // ok, no warning
> > + (void)((void)1, vr); // ok, no warning
> > *tp_fn (); // ok, no warning
> > *vtp_fn (); // ok, no warning
> > tr_fn (); // ok, no warning
> > - vtr_fn (); // ok, no warning{ dg-warning "" } reference
> > not accessed
> > + vtr_fn (); // ok, no warning
>
> Likewise.
>
> > }
> > void extern_test ()
> > @@ -160,5 +160,5 @@ void extern_test ()
> > esr; // ok, no warning
> > vesr; // { dg-warning "" } incomplete not
> > accessed
> > etr; // ok, no warning
> > - vetr; // { dg-warning "" } reference not
> > accessed
> > + vetr; // ok, no warning
> > }
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] c++: convert_to_void and volatile references
2023-09-19 17:53 ` Patrick Palka
@ 2023-09-19 23:27 ` Jason Merrill
0 siblings, 0 replies; 8+ messages in thread
From: Jason Merrill @ 2023-09-19 23:27 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches
On 9/19/23 13:53, Patrick Palka wrote:
> On Mon, 18 Sep 2023, Jason Merrill wrote:
>
>> On 9/18/23 12:12, Patrick Palka wrote:
>>> Jason pointed out that even implicit loads of volatile references need
>>> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
>>> in this case and discard the load. This patch changes this behavior so
>>> that we don't issue a warning, and preserve the load.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> * cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
>>> for an implicit load of a volatile reference. Simplify as if
>>> is_reference is false. Check REFERENCE_REF_P in the test
>>> guarding the -Wunused-value diagnostic.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * g++.dg/expr/discarded1a.C: No longer expect warning for
>>> implicit load of a volatile reference.
>>> * g++.old-deja/g++.bugs/900428_01.C: Likewise.
>>> * g++.dg/expr/volatile2.C: New test.
>>> ---
>>> gcc/cp/cvt.cc | 56 ++-----------------
>>> gcc/testsuite/g++.dg/expr/discarded1a.C | 1 -
>>> gcc/testsuite/g++.dg/expr/volatile2.C | 12 ++++
>>> .../g++.old-deja/g++.bugs/900428_01.C | 26 ++++-----
>>> 4 files changed, 30 insertions(+), 65 deletions(-)
>>> create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
>>>
>>> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
>>> index 4424670356c..1cb6c1222c2 100644
>>> --- a/gcc/cp/cvt.cc
>>> +++ b/gcc/cp/cvt.cc
>>> @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>> {
>>> tree type = TREE_TYPE (expr);
>>> int is_volatile = TYPE_VOLATILE (type);
>>> - if (is_volatile)
>>> - complete_type (type);
>>> - int is_complete = COMPLETE_TYPE_P (type);
>>> /* Can't load the value if we don't know the type. */
>>> - if (is_volatile && !is_complete)
>>> + if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
>>> {
>>> if (complain & tf_warning)
>>> switch (implicit)
>>> @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>> gcc_unreachable ();
>>> }
>>> }
>>> - /* Don't load the value if this is an implicit dereference, or if
>>> - the type needs to be handled by ctors/dtors. */
>>> - else if (is_volatile && is_reference)
>>> - {
>>> - if (complain & tf_warning)
>>> - switch (implicit)
>>> - {
>>> - case ICV_CAST:
>>> - warning_at (loc, 0, "conversion to void will not access "
>>> - "object of type %qT", type);
>>> - break;
>>> - case ICV_SECOND_OF_COND:
>>> - warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> - "object of type %qT in second operand of "
>>> - "conditional expression", type);
>>> - break;
>>> - case ICV_THIRD_OF_COND:
>>> - warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> - "object of type %qT in third operand of "
>>> - "conditional expression", type);
>>> - break;
>>> - case ICV_RIGHT_OF_COMMA:
>>> - warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> - "object of type %qT in right operand of "
>>> - "comma operator", type);
>>> - break;
>>> - case ICV_LEFT_OF_COMMA:
>>> - warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> - "object of type %qT in left operand of comma "
>>> - "operator", type);
>>> - break;
>>> - case ICV_STATEMENT:
>>> - warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> - "object of type %qT in statement", type);
>>> - break;
>>> - case ICV_THIRD_IN_FOR:
>>> - warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> - "object of type %qT in for increment
>>> expression",
>>> - type);
>>> - break;
>>> - default:
>>> - gcc_unreachable ();
>>> - }
>>> - }
>>> + /* Don't load the value if the type needs to be handled by cdtors. */
>>> else if (is_volatile && TREE_ADDRESSABLE (type))
>>> {
>>> if (complain & tf_warning)
>>> @@ -1386,7 +1340,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>> gcc_unreachable ();
>>> }
>>> }
>>> - if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE
>>> (type))
>>> + if (!is_volatile || !COMPLETE_TYPE_P (type))
>>> {
>>> /* Emit a warning (if enabled) when the "effect-less"
>>> INDIRECT_REF
>>> operation is stripped off. Note that we don't warn about
>>> @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>> if (warn_unused_value
>>> && implicit != ICV_CAST
>>> && (complain & tf_warning)
>>> - && !warning_suppressed_p (expr, OPT_Wunused_value)
>>> - && !is_reference)
>>> + && !warning_suppressed_p (expr, OPT_Wunused_value)
>>> + && !REFERENCE_REF_P (expr))
>>> warning_at (loc, OPT_Wunused_value, "value computed is not
>>> used");
>>> expr = TREE_OPERAND (expr, 0);
>>> if (TREE_CODE (expr) == CALL_EXPR
>>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> b/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> index 1c4dff4553e..4754f32f716 100644
>>> --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
>>> int main() {
>>> a; // { dg-message "required from here" }
>>> b; // { dg-message "required from here" }
>>> - // { dg-warning "implicit dereference will not access object" "" { target
>>> *-*-* } .-1 }
>>> *c; // { dg-message "required from here" }
>>> }
>>> diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C
>>> b/gcc/testsuite/g++.dg/expr/volatile2.C
>>> new file mode 100644
>>> index 00000000000..62e609aee2f
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
>>> @@ -0,0 +1,12 @@
>>> +// Verify we preserve implicit loads of volatile references in a
>>> +// discarded-value expression.
>>> +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized"
>>> }
>>> +// { dg-final { scan-tree-dump {\*a} "original" } }
>>> +// { dg-final { scan-tree-dump {\*a} "optimized" } }
>>> +
>>> +struct A { int m; };
>>> +extern volatile A& a;
>>> +
>>> +int main() {
>>> + a;
>>> +}
>>> diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> index a806ef0706e..2b82c64ff73 100644
>>> --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r,
>>> volatile int &vr)
>>> (void)(i ? r : j); // ok, no warning
>>> (void)((void)1, r); // ok, no warning
>>> - vr; // { dg-warning "" } reference not
>>> accessed
>>> - (void)vr; // { dg-warning "" } reference not accessed
>>> - (void)(i ? vj : vr); // { dg-warning "" } reference not
>>> accessed
>>> - (void)(i ? vr : vj); // { dg-warning "" } reference not
>>> accessed
>>> - (void)((void)1, vr); // { dg-warning "" } reference not accessed
>>> + vr; // ok, no warning
>>> + (void)vr; // ok, no warning
>>> + (void)(i ? vj : vr); // ok, no warning
>>> + (void)(i ? vr : vj); // ok, no warning
>>> + (void)((void)1, vr); // ok, no warning
>>> *ip_fn (); // ok, no warning
>>> *vip_fn (); // ok, no warning
>>> ir_fn (); // ok, no warning
>>> - vir_fn (); // { dg-warning "" } reference not accessed
>>> + vir_fn (); // ok, no warning
>>
>> This should still warn (and not load), a function call is not one of the forms
>> in https://eel.is/c++draft/expr#context-2. We still need the warnings you're
>> removing for expression forms that aren't on that list.
>
> D'oh, will fix. It's surprising at first glance that *vip_fn() behaves
> differently from vir_fn().
Yeah, the rationale was that in *vip_fn the last explicit semantic is
the dereference, while in vir_fn() it's the call. It is pretty subtle,
though.
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-09-19 23:27 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-18 16:12 [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
2023-09-18 16:12 ` [PATCH v2 2/2] c++: convert_to_void and volatile references Patrick Palka
2023-09-18 16:31 ` Patrick Palka
2023-09-19 1:44 ` Jason Merrill
2023-09-19 17:53 ` Patrick Palka
2023-09-19 23:27 ` Jason Merrill
2023-09-18 16:19 ` [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419] Patrick Palka
2023-09-19 1:45 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).