Re: [PATCH] [PR86823] retain deferred access checks from outside firewall
OK.
On Thu, Dec 13, 2018 at 8:35 PM Alexandre Oliva wrote:
>
> On Dec 6, 2018, Alexandre Oliva wrote:
>
> > I'm giving your proposed patch a full round of testing along with other
> > patches.
>
> [PR86823] retain deferred access checks from outside firewall
>
> We used to preserve deferred access check along with resolved template
> ids, but a tentative parsing firewall introduced additional layers of
> deferred access checks, so that we don't preserve the checks we
> want to any more.
>
> This patch moves the deferred access checks from outside the firewall
> into it.
>
> Regstrapped on x86_64- and i686-linux-gnu. Ok to install?
>
>
> From: Jason Merrill
> for gcc/cp/ChangeLog
>
> PR c++/86823
> * parser.c (cp_parser_template_id): Rearrange deferred access
> checks into the firewall.
>
> From: Alexandre Oliva
> for gcc/testsuite/ChangeLog
>
> PR c++/86823
> * g++.dg/pr86823.C: New.
> ---
> gcc/cp/parser.c| 10 ++
> gcc/testsuite/g++.dg/pr86823.C | 15 +++
> 2 files changed, 21 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/pr86823.C
>
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index adfe09e494dc..0bf0e309a588 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -16182,16 +16182,18 @@ cp_parser_template_id (cp_parser *parser,
>is_declaration,
>tag_type,
>_identifier);
> +
> + /* Push any access checks inside the firewall we're about to create. */
> + vec *checks = get_deferred_access_checks ();
> + pop_deferring_access_checks ();
>if (templ == error_mark_node || is_identifier)
> -{
> - pop_deferring_access_checks ();
> - return templ;
> -}
> +return templ;
>
>/* Since we're going to preserve any side-effects from this parse, set up a
> firewall to protect our callers from cp_parser_commit_to_tentative_parse
> in the template arguments. */
>tentative_firewall firewall (parser);
> + reopen_deferring_access_checks (checks);
>
>/* If we find the sequence `[:' after a template-name, it's probably
> a digraph-typo for `< ::'. Substitute the tokens and check if we can
> diff --git a/gcc/testsuite/g++.dg/pr86823.C b/gcc/testsuite/g++.dg/pr86823.C
> new file mode 100644
> index ..18914b00aa8d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr86823.C
> @@ -0,0 +1,15 @@
> +// { dg-do compile }
> +
> +struct X {
> +private:
> + template
> + struct Y {
> +int data;
> + };
> +public:
> + int value;
> +};
> +
> +int main() {
> + typename X::Y a; // { dg-error "private" }
> +}
>
>
> --
> Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
> Be the change, be Free! FSF Latin America board member
> GNU Toolchain EngineerFree Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [PATCH] [PR86823] retain deferred access checks from outside firewall
On Dec 6, 2018, Alexandre Oliva wrote:
> I'm giving your proposed patch a full round of testing along with other
> patches.
[PR86823] retain deferred access checks from outside firewall
We used to preserve deferred access check along with resolved template
ids, but a tentative parsing firewall introduced additional layers of
deferred access checks, so that we don't preserve the checks we
want to any more.
This patch moves the deferred access checks from outside the firewall
into it.
Regstrapped on x86_64- and i686-linux-gnu. Ok to install?
From: Jason Merrill
for gcc/cp/ChangeLog
PR c++/86823
* parser.c (cp_parser_template_id): Rearrange deferred access
checks into the firewall.
From: Alexandre Oliva
for gcc/testsuite/ChangeLog
PR c++/86823
* g++.dg/pr86823.C: New.
---
gcc/cp/parser.c| 10 ++
gcc/testsuite/g++.dg/pr86823.C | 15 +++
2 files changed, 21 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/pr86823.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index adfe09e494dc..0bf0e309a588 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -16182,16 +16182,18 @@ cp_parser_template_id (cp_parser *parser,
is_declaration,
tag_type,
_identifier);
+
+ /* Push any access checks inside the firewall we're about to create. */
+ vec *checks = get_deferred_access_checks ();
+ pop_deferring_access_checks ();
if (templ == error_mark_node || is_identifier)
-{
- pop_deferring_access_checks ();
- return templ;
-}
+return templ;
/* Since we're going to preserve any side-effects from this parse, set up a
firewall to protect our callers from cp_parser_commit_to_tentative_parse
in the template arguments. */
tentative_firewall firewall (parser);
+ reopen_deferring_access_checks (checks);
/* If we find the sequence `[:' after a template-name, it's probably
a digraph-typo for `< ::'. Substitute the tokens and check if we can
diff --git a/gcc/testsuite/g++.dg/pr86823.C b/gcc/testsuite/g++.dg/pr86823.C
new file mode 100644
index ..18914b00aa8d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr86823.C
@@ -0,0 +1,15 @@
+// { dg-do compile }
+
+struct X {
+private:
+ template
+ struct Y {
+int data;
+ };
+public:
+ int value;
+};
+
+int main() {
+ typename X::Y a; // { dg-error "private" }
+}
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [PATCH] [PR86823] retain deferred access checks from outside firewall
On Dec 5, 2018, Jason Merrill wrote: > Hmm, I'm uncomfortable with how this depends on the specific > implementation of tentative_firewall. *nod* > What do you think of this alternate approach (untested other than with > the testcase)? If that won't drop any other deferred access checks that we might want to keep when leaving the firewall by other paths, it looks much nicer indeed! I had been careful to only move access checks when I was certain about the replacement, but even there I was concerned about carrying stuff along with the preparsed token that didn't belong. I'm giving your proposed patch a full round of testing along with other patches. -- Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo Be the change, be Free! FSF Latin America board member GNU Toolchain EngineerFree Software Evangelist Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [PATCH] [PR86823] retain deferred access checks from outside firewall
On 11/16/18 9:16 PM, Alexandre Oliva wrote:
We used to preserve deferred access check along with resolved template
ids, but a tentative parsing firewall introduced additional layers of
deferred access checks, so that we don't preserve the checks we
want to any more.
This patch collapses the access check levels introduced by the
firewall with those we wanted to preserve to begin with, saves them,
pushes them one more layer up so that the firewall doesn't drop them,
and then pushes new empty levels so balance out with the upcoming
popping.
We may want to avoid the pop_to_parent and one push after the save, and
instead leave the firewall's scope before the final pop_to_parent.
However, this patch is what I regression-tested successfully with
check-g++ on x86_64-linux-gnu. Regstrapping on i686- and
x86_64-linux-gnu now. Ok to install?
for gcc/cp/ChangeLog
PR c++/86823
* parser.c (cp_parser_template_id): Merge access checks from
outside the tentative parsing firewall with those from inside,
and save them all with the template id token.
for gcc/testsuite/ChangeLog
PR c++/86823
* g++.dg/pr86823.C: New.
---
gcc/cp/parser.c| 12
gcc/testsuite/g++.dg/pr86823.C | 15 +++
2 files changed, 27 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/pr86823.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index db0f0338179e..6a08b09715a7 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -16185,7 +16185,19 @@ cp_parser_template_id (cp_parser *parser,
so the memory will not be reclaimed during token replacing below. */
token->u.tree_check_value = ggc_cleared_alloc ();
token->u.tree_check_value->value = template_id;
+ /* Collapse the access check levels introduced by
+tentative_firewall with the one we introduced ourselves. */
+ pop_to_parent_deferring_access_checks ();
+ pop_to_parent_deferring_access_checks ();
token->u.tree_check_value->checks = get_deferred_access_checks ();
+ /* Push the checks up one more level, so that the firewall
+doesn't drop them on the floor when we return. Then, push
+empty levels back in place so that they are popped
+properly. */
+ pop_to_parent_deferring_access_checks ();
+ push_deferring_access_checks (dk_deferred);
+ push_deferring_access_checks (dk_deferred);
+ push_deferring_access_checks (dk_deferred);
Hmm, I'm uncomfortable with how this depends on the specific
implementation of tentative_firewall. What do you think of this
alternate approach (untested other than with the testcase)?
Jason
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index ac19cb4b9bb..c7ec7dcf413 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -16182,16 +16182,18 @@ cp_parser_template_id (cp_parser *parser,
is_declaration,
tag_type,
_identifier);
+
+ /* Push any access checks inside the firewall we're about to create. */
+ vec *checks = get_deferred_access_checks ();
+ pop_deferring_access_checks ();
if (templ == error_mark_node || is_identifier)
-{
- pop_deferring_access_checks ();
- return templ;
-}
+return templ;
/* Since we're going to preserve any side-effects from this parse, set up a
firewall to protect our callers from cp_parser_commit_to_tentative_parse
in the template arguments. */
tentative_firewall firewall (parser);
+ reopen_deferring_access_checks (checks);
/* If we find the sequence `[:' after a template-name, it's probably
a digraph-typo for `< ::'. Substitute the tokens and check if we can
[PATCH] [PR86823] retain deferred access checks from outside firewall
We used to preserve deferred access check along with resolved template
ids, but a tentative parsing firewall introduced additional layers of
deferred access checks, so that we don't preserve the checks we
want to any more.
This patch collapses the access check levels introduced by the
firewall with those we wanted to preserve to begin with, saves them,
pushes them one more layer up so that the firewall doesn't drop them,
and then pushes new empty levels so balance out with the upcoming
popping.
We may want to avoid the pop_to_parent and one push after the save, and
instead leave the firewall's scope before the final pop_to_parent.
However, this patch is what I regression-tested successfully with
check-g++ on x86_64-linux-gnu. Regstrapping on i686- and
x86_64-linux-gnu now. Ok to install?
for gcc/cp/ChangeLog
PR c++/86823
* parser.c (cp_parser_template_id): Merge access checks from
outside the tentative parsing firewall with those from inside,
and save them all with the template id token.
for gcc/testsuite/ChangeLog
PR c++/86823
* g++.dg/pr86823.C: New.
---
gcc/cp/parser.c| 12
gcc/testsuite/g++.dg/pr86823.C | 15 +++
2 files changed, 27 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/pr86823.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index db0f0338179e..6a08b09715a7 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -16185,7 +16185,19 @@ cp_parser_template_id (cp_parser *parser,
so the memory will not be reclaimed during token replacing below. */
token->u.tree_check_value = ggc_cleared_alloc ();
token->u.tree_check_value->value = template_id;
+ /* Collapse the access check levels introduced by
+tentative_firewall with the one we introduced ourselves. */
+ pop_to_parent_deferring_access_checks ();
+ pop_to_parent_deferring_access_checks ();
token->u.tree_check_value->checks = get_deferred_access_checks ();
+ /* Push the checks up one more level, so that the firewall
+doesn't drop them on the floor when we return. Then, push
+empty levels back in place so that they are popped
+properly. */
+ pop_to_parent_deferring_access_checks ();
+ push_deferring_access_checks (dk_deferred);
+ push_deferring_access_checks (dk_deferred);
+ push_deferring_access_checks (dk_deferred);
token->keyword = RID_MAX;
/* Purge all subsequent tokens. */
diff --git a/gcc/testsuite/g++.dg/pr86823.C b/gcc/testsuite/g++.dg/pr86823.C
new file mode 100644
index ..18914b00aa8d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr86823.C
@@ -0,0 +1,15 @@
+// { dg-do compile }
+
+struct X {
+private:
+ template
+ struct Y {
+int data;
+ };
+public:
+ int value;
+};
+
+int main() {
+ typename X::Y a; // { dg-error "private" }
+}
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
