[C++] PR c++/90449 - add -Winaccessible-base option.

Message ID CAOrE4X2UYzhe8DFd_ntqCzz2gcMF+h0iYo8bQtkaRPyKGO0VUA@mail.gmail.com
State New
Headers show
Series
  • [C++] PR c++/90449 - add -Winaccessible-base option.
Related show

Commit Message

Matthew Beliveau June 7, 2019, 8:10 p.m.
This patch adds a new warning option: Winaccessible-base, so that
users are able to selectively control the warning. The warning is
enabled by default; however, for the virtual bases' warning, it only
triggers with -Wextra flag.

Comments

Paolo Carlini June 7, 2019, 8:20 p.m. | #1
Hi,

On 07/06/19 22:10, Matthew Beliveau wrote:
> @@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)


Just a nit, but it seems weird to me that the function implementing 
warn_inaccessible_base is named warn_about_ambiguous_bases.

Paolo.
Marek Polacek June 7, 2019, 8:31 p.m. | #2
On Fri, Jun 07, 2019 at 10:20:02PM +0200, Paolo Carlini wrote:
> Hi,

> 

> On 07/06/19 22:10, Matthew Beliveau wrote:

> > @@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)

> 

> Just a nit, but it seems weird to me that the function implementing

> warn_inaccessible_base is named warn_about_ambiguous_bases.


Maybe, but we want to keep the warning's name in sync with clang, and
renaming the function seems like unnecessary noise.  I could go either
way.

Marek
Paolo Carlini June 7, 2019, 8:42 p.m. | #3
Hi,

On 07/06/19 22:31, Marek Polacek wrote:
> On Fri, Jun 07, 2019 at 10:20:02PM +0200, Paolo Carlini wrote:

>> Hi,

>>

>> On 07/06/19 22:10, Matthew Beliveau wrote:

>>> @@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)

>> Just a nit, but it seems weird to me that the function implementing

>> warn_inaccessible_base is named warn_about_ambiguous_bases.

> Maybe, but we want to keep the warning's name in sync with clang, and

> renaming the function seems like unnecessary noise.  I could go either

> way.


It's definitely a minor issue. But, given that we have a rationale for 
inaccessible_base - I didn't know that - I vote for renaming the 
function. A maybe_* name would be appropriate in that case, because the 
function doesn't always warn.

Paolo.
Matthew Beliveau June 10, 2019, 2:19 p.m. | #4
Hello,

I've changed the name of warn_about_ambiguous_bases to
maybe_warn_about_inaccessible_bases.

I've attached the new patch below.

Best,
Matthew Beliveau

On Fri, Jun 7, 2019 at 4:42 PM Paolo Carlini <paolo.carlini@oracle.com> wrote:
>

> Hi,

>

> On 07/06/19 22:31, Marek Polacek wrote:

> > On Fri, Jun 07, 2019 at 10:20:02PM +0200, Paolo Carlini wrote:

> >> Hi,

> >>

> >> On 07/06/19 22:10, Matthew Beliveau wrote:

> >>> @@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)

> >> Just a nit, but it seems weird to me that the function implementing

> >> warn_inaccessible_base is named warn_about_ambiguous_bases.

> > Maybe, but we want to keep the warning's name in sync with clang, and

> > renaming the function seems like unnecessary noise.  I could go either

> > way.

>

> It's definitely a minor issue. But, given that we have a rationale for

> inaccessible_base - I didn't know that - I vote for renaming the

> function. A maybe_* name would be appropriate in that case, because the

> function doesn't always warn.

>

> Paolo.

>
Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-06-10  Matthew Beliveau  <mbelivea@redhat.com>

       PR c++/90449 - add -Winaccessible-base option.
       * doc/invoke.texi (Winaccessible-base): Document.

       * c.opt (Winaccessible-base): Added new option.

       * class.c (warn_about_ambiguous_bases): Changed name to:
       maybe_warn_about_inaccessible_bases.
       (maybe_warn_about_inaccessible_bases):  Implemented new
       Winaccessible-base warning option for both direct and virtual
       base warnings.
       (layout_class_type): Call to warn_about_ambiguous_bases changed to fit
       new name.

       	* g++.dg/warn/Winaccessible-base-1.C: New file.
       	* g++.dg/warn/Winaccessible-base-2.C: New file.
       	* g++.dg/warn/Winaccessible-virtual-base-1.C: New file.
       	* g++.dg/warn/Winaccessible-virtual-base-2.C: New file.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 046d489f7eb..144f6da15d6 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -625,6 +625,10 @@ Wignored-attributes
 C C++ Var(warn_ignored_attributes) Init(1) Warning
 Warn whenever attributes are ignored.

+Winaccessible-base
+C++ ObjC++ Var(warn_inaccessible_base) Init(1) Warning
+Warn when a base is inaccessible in derived due to ambiguity.
+
 Wincompatible-pointer-types
 C ObjC Var(warn_incompatible_pointer_types) Init(1) Warning
 Warn when there is a conversion between pointers that have incompatible types.
diff --git gcc/cp/class.c gcc/cp/class.c
index a2585a61f96..9884cedc997 100644
--- gcc/cp/class.c
+++ gcc/cp/class.c
@@ -199,7 +199,7 @@ static int walk_subobject_offsets (tree, subobject_offset_fn,
 static int layout_conflict_p (tree, tree, splay_tree, int);
 static int splay_tree_compare_integer_csts (splay_tree_key k1,
 					    splay_tree_key k2);
-static void warn_about_ambiguous_bases (tree);
+static void maybe_warn_about_inaccessible_bases (tree);
 static bool type_requires_array_cookie (tree);
 static bool base_derived_from (tree, tree);
 static int empty_base_at_nonzero_offset_p (tree, tree, splay_tree);
@@ -6017,7 +6017,7 @@ end_of_class (tree t, bool include_virtuals_p)
    subobjects of U.  */

 static void
-warn_about_ambiguous_bases (tree t)
+maybe_warn_about_inaccessible_bases (tree t)
 {
   int i;
   vec<tree, va_gc> *vbases;
@@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)
   tree binfo;
   tree base_binfo;

+  /* If not checking for warning then return early.  */
+  if (!warn_inaccessible_base)
+    return;
+
   /* If there are no repeated bases, nothing can be ambiguous.  */
   if (!CLASSTYPE_REPEATED_BASE_P (t))
     return;
@@ -6036,8 +6040,8 @@ warn_about_ambiguous_bases (tree t)
       basetype = BINFO_TYPE (base_binfo);

       if (!uniquely_derived_from_p (basetype, t))
-	warning (0, "direct base %qT inaccessible in %qT due to ambiguity",
-		 basetype, t);
+	warning (OPT_Winaccessible_base, "direct base %qT inaccessible "
+		 "in %qT due to ambiguity", basetype, t);
     }

   /* Check for ambiguous virtual bases.  */
@@ -6048,8 +6052,8 @@ warn_about_ambiguous_bases (tree t)
 	basetype = BINFO_TYPE (binfo);

 	if (!uniquely_derived_from_p (basetype, t))
-	  warning (OPT_Wextra, "virtual base %qT inaccessible in %qT due "
-		   "to ambiguity", basetype, t);
+	  warning (OPT_Winaccessible_base, "virtual base %qT inaccessible in "
+		   "%qT due to ambiguity", basetype, t);
       }
 }

@@ -6455,7 +6459,7 @@ layout_class_type (tree t, tree *virtuals_p)
     error ("size of type %qT is too large (%qE bytes)", t, TYPE_SIZE_UNIT (t));

   /* Warn about bases that can't be talked about due to ambiguity.  */
-  warn_about_ambiguous_bases (t);
+  maybe_warn_about_inaccessible_bases (t);

   /* Now that we're done with layout, give the base fields the real types.  */
   for (field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 3e4f012b4fa..862ee794773 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -317,6 +317,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wignored-qualifiers  -Wignored-attributes  -Wincompatible-pointer-types @gol
 -Wimplicit  -Wimplicit-fallthrough  -Wimplicit-fallthrough=@var{n} @gol
 -Wimplicit-function-declaration  -Wimplicit-int @gol
+-Winaccessible-base @gol
 -Winit-self  -Winline  -Wno-int-conversion  -Wint-in-bool-context @gol
 -Wno-int-to-pointer-cast  -Winvalid-memory-model  -Wno-invalid-offsetof @gol
 -Winvalid-pch  -Wlarger-than=@var{byte-size} @gol
@@ -4800,6 +4801,22 @@ is only active when @option{-fdelete-null-pointer-checks} is active,
 which is enabled by optimizations in most targets.  The precision of
 the warnings depends on the optimization options used.

+@item -Winaccessible-base @r{(C++, Objective-C++ only)}
+@opindex Winaccessible-base
+@opindex Wno-inaccessible-base
+Warn when a base is inaccessible in derived due to ambiguity.  The warning is
+enabled by default.  Note the warning for virtual bases is enabled by the
+@option{-Wextra} option.
+@smallexample
+@group
+struct A @{ int a; @};
+
+struct B : A @{ @};
+
+struct C : B, A @{ @};
+@end group
+@end smallexample
+
 @item -Winit-self @r{(C, C++, Objective-C and Objective-C++ only)}
 @opindex Winit-self
 @opindex Wno-init-self
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C
new file mode 100644
index 00000000000..2e32b0b119f
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C
@@ -0,0 +1,7 @@
+// PR c++/90449
+
+struct A { int a; };
+
+struct B : A { };
+
+struct C : B, A { }; // { dg-warning "direct base 'A' inaccessible in 'C' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C
new file mode 100644
index 00000000000..67bd740a763
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C
@@ -0,0 +1,8 @@
+// PR c++/90449
+// { dg-options -Wno-inaccessible-base }
+
+struct A { int a; };
+
+struct B : A { };
+
+struct C : B, A { }; // { dg-bogus "direct base 'A' inaccessible in 'C' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C
new file mode 100644
index 00000000000..051fcc695fe
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C
@@ -0,0 +1,10 @@
+// PR c++/90449
+// { dg-options -Wextra }
+
+struct A { };
+
+struct B : virtual A { };
+
+struct C : A { };
+
+struct D : B, C { }; // { dg-warning "virtual base 'A' inaccessible in 'D' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C
new file mode 100644
index 00000000000..eab9ec0e3d7
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C
@@ -0,0 +1,10 @@
+// PR c++/90449
+// { dg-options "-Wextra -Wno-inaccessible-base" }
+
+struct A { };
+
+struct B : virtual A { };
+
+struct C : A { };
+
+struct D : B, C { }; // { dg-bogus "virtual base 'A' inaccessible in 'D' due to ambiguity" }
Marek Polacek June 10, 2019, 2:49 p.m. | #5
On Mon, Jun 10, 2019 at 10:19:51AM -0400, Matthew Beliveau wrote:
> Hello,

> 

> I've changed the name of warn_about_ambiguous_bases to

> maybe_warn_about_inaccessible_bases.

> 

> I've attached the new patch below.

> 

> Best,

> Matthew Beliveau

> 

> On Fri, Jun 7, 2019 at 4:42 PM Paolo Carlini <paolo.carlini@oracle.com> wrote:

> >

> > Hi,

> >

> > On 07/06/19 22:31, Marek Polacek wrote:

> > > On Fri, Jun 07, 2019 at 10:20:02PM +0200, Paolo Carlini wrote:

> > >> Hi,

> > >>

> > >> On 07/06/19 22:10, Matthew Beliveau wrote:

> > >>> @@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)

> > >> Just a nit, but it seems weird to me that the function implementing

> > >> warn_inaccessible_base is named warn_about_ambiguous_bases.

> > > Maybe, but we want to keep the warning's name in sync with clang, and

> > > renaming the function seems like unnecessary noise.  I could go either

> > > way.

> >

> > It's definitely a minor issue. But, given that we have a rationale for

> > inaccessible_base - I didn't know that - I vote for renaming the

> > function. A maybe_* name would be appropriate in that case, because the

> > function doesn't always warn.

> >

> > Paolo.

> >


> Bootstrapped/regtested on x86_64-linux, ok for trunk?

> 

> 2019-06-10  Matthew Beliveau  <mbelivea@redhat.com>

> 

>        PR c++/90449 - add -Winaccessible-base option.

>        * doc/invoke.texi (Winaccessible-base): Document.

> 

>        * c.opt (Winaccessible-base): Added new option.

> 

>        * class.c (warn_about_ambiguous_bases): Changed name to:

>        maybe_warn_about_inaccessible_bases.

>        (maybe_warn_about_inaccessible_bases):  Implemented new

>        Winaccessible-base warning option for both direct and virtual

>        base warnings.

>        (layout_class_type): Call to warn_about_ambiguous_bases changed to fit

>        new name.

> 

>        	* g++.dg/warn/Winaccessible-base-1.C: New file.

>        	* g++.dg/warn/Winaccessible-base-2.C: New file.

>        	* g++.dg/warn/Winaccessible-virtual-base-1.C: New file.

>        	* g++.dg/warn/Winaccessible-virtual-base-2.C: New file.


Looks fine to me except the small issues with whitespaces in the CL entry.

Marek
Martin Sebor June 10, 2019, 3:39 p.m. | #6
On 6/7/19 2:10 PM, Matthew Beliveau wrote:
> This patch adds a new warning option: Winaccessible-base, so that

> users are able to selectively control the warning. The warning is

> enabled by default; however, for the virtual bases' warning, it only

> triggers with -Wextra flag.


With few exceptions the rest of the manual tends to refer to base
classes.  I would suggest to do the same in this update as well:

@@ -4800,6 +4801,22 @@ is only active when 
@option{-fdelete-null-pointer-checks} is active,
  which is enabled by optimizations in most targets.  The precision of
  the warnings depends on the optimization options used.

+@item -Winaccessible-base @r{(C++, Objective-C++ only)}
+@opindex Winaccessible-base
+@opindex Wno-inaccessible-base
+Warn when a base is inaccessible in derived due to ambiguity.  The 
warning is
+enabled by default.  Note the warning for virtual bases is enabled by the
+@option{-Wextra} option.


I.e.,

   Warn when a base class is inaccessible in a class derived from
   it due to ambiguity.

Martin
Matthew Beliveau June 10, 2019, 4:02 p.m. | #7
Hello,

i changed the doc/invoke.texi like you suggested. Let me know If I
missed anything!

Thank you,
Matthew Beliveau

On Mon, Jun 10, 2019 at 11:39 AM Martin Sebor <msebor@gmail.com> wrote:
>

> On 6/7/19 2:10 PM, Matthew Beliveau wrote:

> > This patch adds a new warning option: Winaccessible-base, so that

> > users are able to selectively control the warning. The warning is

> > enabled by default; however, for the virtual bases' warning, it only

> > triggers with -Wextra flag.

>

> With few exceptions the rest of the manual tends to refer to base

> classes.  I would suggest to do the same in this update as well:

>

> @@ -4800,6 +4801,22 @@ is only active when

> @option{-fdelete-null-pointer-checks} is active,

>   which is enabled by optimizations in most targets.  The precision of

>   the warnings depends on the optimization options used.

>

> +@item -Winaccessible-base @r{(C++, Objective-C++ only)}

> +@opindex Winaccessible-base

> +@opindex Wno-inaccessible-base

> +Warn when a base is inaccessible in derived due to ambiguity.  The

> warning is

> +enabled by default.  Note the warning for virtual bases is enabled by the

> +@option{-Wextra} option.

>

>

> I.e.,

>

>    Warn when a base class is inaccessible in a class derived from

>    it due to ambiguity.

>

> Martin
Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-06-10  Matthew Beliveau  <mbelivea@redhat.com>

       PR c++/90449 - add -Winaccessible-base option.
       * doc/invoke.texi (Winaccessible-base): Document.

       * c.opt (Winaccessible-base): Added new option.

       * class.c (warn_about_ambiguous_bases): Changed name to:
       maybe_warn_about_inaccessible_bases.
       (maybe_warn_about_inaccessible_bases):  Implemented new
       Winaccessible-base warning option for both direct and virtual
       base warnings.
       (layout_class_type): Call to warn_about_ambiguous_bases changed to fit
       new name.

       	* g++.dg/warn/Winaccessible-base-1.C: New file.
       	* g++.dg/warn/Winaccessible-base-2.C: New file.
       	* g++.dg/warn/Winaccessible-virtual-base-1.C: New file.
       	* g++.dg/warn/Winaccessible-virtual-base-2.C: New file.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 046d489f7eb..144f6da15d6 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -625,6 +625,10 @@ Wignored-attributes
 C C++ Var(warn_ignored_attributes) Init(1) Warning
 Warn whenever attributes are ignored.

+Winaccessible-base
+C++ ObjC++ Var(warn_inaccessible_base) Init(1) Warning
+Warn when a base is inaccessible in derived due to ambiguity.
+
 Wincompatible-pointer-types
 C ObjC Var(warn_incompatible_pointer_types) Init(1) Warning
 Warn when there is a conversion between pointers that have incompatible types.
diff --git gcc/cp/class.c gcc/cp/class.c
index a2585a61f96..9884cedc997 100644
--- gcc/cp/class.c
+++ gcc/cp/class.c
@@ -199,7 +199,7 @@ static int walk_subobject_offsets (tree, subobject_offset_fn,
 static int layout_conflict_p (tree, tree, splay_tree, int);
 static int splay_tree_compare_integer_csts (splay_tree_key k1,
 					    splay_tree_key k2);
-static void warn_about_ambiguous_bases (tree);
+static void maybe_warn_about_inaccessible_bases (tree);
 static bool type_requires_array_cookie (tree);
 static bool base_derived_from (tree, tree);
 static int empty_base_at_nonzero_offset_p (tree, tree, splay_tree);
@@ -6017,7 +6017,7 @@ end_of_class (tree t, bool include_virtuals_p)
    subobjects of U.  */

 static void
-warn_about_ambiguous_bases (tree t)
+maybe_warn_about_inaccessible_bases (tree t)
 {
   int i;
   vec<tree, va_gc> *vbases;
@@ -6025,6 +6025,10 @@ warn_about_ambiguous_bases (tree t)
   tree binfo;
   tree base_binfo;

+  /* If not checking for warning then return early.  */
+  if (!warn_inaccessible_base)
+    return;
+
   /* If there are no repeated bases, nothing can be ambiguous.  */
   if (!CLASSTYPE_REPEATED_BASE_P (t))
     return;
@@ -6036,8 +6040,8 @@ warn_about_ambiguous_bases (tree t)
       basetype = BINFO_TYPE (base_binfo);

       if (!uniquely_derived_from_p (basetype, t))
-	warning (0, "direct base %qT inaccessible in %qT due to ambiguity",
-		 basetype, t);
+	warning (OPT_Winaccessible_base, "direct base %qT inaccessible "
+		 "in %qT due to ambiguity", basetype, t);
     }

   /* Check for ambiguous virtual bases.  */
@@ -6048,8 +6052,8 @@ warn_about_ambiguous_bases (tree t)
 	basetype = BINFO_TYPE (binfo);

 	if (!uniquely_derived_from_p (basetype, t))
-	  warning (OPT_Wextra, "virtual base %qT inaccessible in %qT due "
-		   "to ambiguity", basetype, t);
+	  warning (OPT_Winaccessible_base, "virtual base %qT inaccessible in "
+		   "%qT due to ambiguity", basetype, t);
       }
 }

@@ -6455,7 +6459,7 @@ layout_class_type (tree t, tree *virtuals_p)
     error ("size of type %qT is too large (%qE bytes)", t, TYPE_SIZE_UNIT (t));

   /* Warn about bases that can't be talked about due to ambiguity.  */
-  warn_about_ambiguous_bases (t);
+  maybe_warn_about_inaccessible_bases (t);

   /* Now that we're done with layout, give the base fields the real types.  */
   for (field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 3e4f012b4fa..2877e7c69a1 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -317,6 +317,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wignored-qualifiers  -Wignored-attributes  -Wincompatible-pointer-types @gol
 -Wimplicit  -Wimplicit-fallthrough  -Wimplicit-fallthrough=@var{n} @gol
 -Wimplicit-function-declaration  -Wimplicit-int @gol
+-Winaccessible-base @gol
 -Winit-self  -Winline  -Wno-int-conversion  -Wint-in-bool-context @gol
 -Wno-int-to-pointer-cast  -Winvalid-memory-model  -Wno-invalid-offsetof @gol
 -Winvalid-pch  -Wlarger-than=@var{byte-size} @gol
@@ -4800,6 +4801,22 @@ is only active when @option{-fdelete-null-pointer-checks} is active,
 which is enabled by optimizations in most targets.  The precision of
 the warnings depends on the optimization options used.

+@item -Winaccessible-base @r{(C++, Objective-C++ only)}
+@opindex Winaccessible-base
+@opindex Wno-inaccessible-base
+Warn when a base class is inaccessible in a class derived from it due to
+ambiguity.  The warning is enabled by default.  Note the warning for virtual
+bases is enabled by the @option{-Wextra} option.
+@smallexample
+@group
+struct A @{ int a; @};
+
+struct B : A @{ @};
+
+struct C : B, A @{ @};
+@end group
+@end smallexample
+
 @item -Winit-self @r{(C, C++, Objective-C and Objective-C++ only)}
 @opindex Winit-self
 @opindex Wno-init-self
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C
new file mode 100644
index 00000000000..2e32b0b119f
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C
@@ -0,0 +1,7 @@
+// PR c++/90449
+
+struct A { int a; };
+
+struct B : A { };
+
+struct C : B, A { }; // { dg-warning "direct base 'A' inaccessible in 'C' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C
new file mode 100644
index 00000000000..67bd740a763
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C
@@ -0,0 +1,8 @@
+// PR c++/90449
+// { dg-options -Wno-inaccessible-base }
+
+struct A { int a; };
+
+struct B : A { };
+
+struct C : B, A { }; // { dg-bogus "direct base 'A' inaccessible in 'C' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C
new file mode 100644
index 00000000000..051fcc695fe
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C
@@ -0,0 +1,10 @@
+// PR c++/90449
+// { dg-options -Wextra }
+
+struct A { };
+
+struct B : virtual A { };
+
+struct C : A { };
+
+struct D : B, C { }; // { dg-warning "virtual base 'A' inaccessible in 'D' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C
new file mode 100644
index 00000000000..eab9ec0e3d7
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C
@@ -0,0 +1,10 @@
+// PR c++/90449
+// { dg-options "-Wextra -Wno-inaccessible-base" }
+
+struct A { };
+
+struct B : virtual A { };
+
+struct C : A { };
+
+struct D : B, C { }; // { dg-bogus "virtual base 'A' inaccessible in 'D' due to ambiguity" }
Jason Merrill June 11, 2019, 4:43 a.m. | #8
On 6/10/19 12:02 PM, Matthew Beliveau wrote:
>   	if (!uniquely_derived_from_p (basetype, t))

> -	  warning (OPT_Wextra, "virtual base %qT inaccessible in %qT due "

> -		   "to ambiguity", basetype, t);

> +	  warning (OPT_Winaccessible_base, "virtual base %qT inaccessible in "

> +		   "%qT due to ambiguity", basetype, t);


You mentioned using -Wextra for this message, and you have a testcase 
for it, but here you remove the only connection between this message and 
-Wextra: with this patch, the virtual base warning will be enabled by 
default.  Perhaps you want to check extra_warnings in the if condition?

Jason
Matthew Beliveau June 11, 2019, 1:59 p.m. | #9
Hello,

Correct me if I'm wrong, but before the function checks for virtual
bases, an if-statement checks for extra_warnings on line 6049(when my
patch is applied). The check was there before I made my changes, and
my test fails
without  -Wextra.  Below is the code above the warning call:

  if (extra_warnings)
    for (vbases = CLASSTYPE_VBASECLASSES (t), i = 0;
vec_safe_iterate (vbases, i, &binfo); i++)
      {
basetype = BINFO_TYPE (binfo);

Unless I'm mistaken and you meant for me to check for extra_warnings
again in the if-statement directly above the warning call.

Thank you,
Matthew Beliveau

On Tue, Jun 11, 2019 at 12:43 AM Jason Merrill <jason@redhat.com> wrote:
>

> On 6/10/19 12:02 PM, Matthew Beliveau wrote:

> >       if (!uniquely_derived_from_p (basetype, t))

> > -       warning (OPT_Wextra, "virtual base %qT inaccessible in %qT due "

> > -                "to ambiguity", basetype, t);

> > +       warning (OPT_Winaccessible_base, "virtual base %qT inaccessible in "

> > +                "%qT due to ambiguity", basetype, t);

>

> You mentioned using -Wextra for this message, and you have a testcase

> for it, but here you remove the only connection between this message and

> -Wextra: with this patch, the virtual base warning will be enabled by

> default.  Perhaps you want to check extra_warnings in the if condition?

>

> Jason
Jason Merrill June 11, 2019, 2:33 p.m. | #10
On 6/11/19 9:59 AM, Matthew Beliveau wrote:
> Correct me if I'm wrong, but before the function checks for virtual

> bases, an if-statement checks for extra_warnings on line 6049(when my

> patch is applied). The check was there before I made my changes, and

> my test fails without  -Wextra.  Below is the code above the warning call:

> 

>    if (extra_warnings)

>      for (vbases = CLASSTYPE_VBASECLASSES (t), i = 0;

> vec_safe_iterate (vbases, i, &binfo); i++)


Ah, yes, thanks, I should have looked at the wider context.  The patch 
is OK.

Jason
Marek Polacek June 11, 2019, 3:04 p.m. | #11
On Tue, Jun 11, 2019 at 10:33:07AM -0400, Jason Merrill wrote:
> On 6/11/19 9:59 AM, Matthew Beliveau wrote:

> > Correct me if I'm wrong, but before the function checks for virtual

> > bases, an if-statement checks for extra_warnings on line 6049(when my

> > patch is applied). The check was there before I made my changes, and

> > my test fails without  -Wextra.  Below is the code above the warning call:

> > 

> >    if (extra_warnings)

> >      for (vbases = CLASSTYPE_VBASECLASSES (t), i = 0;

> > vec_safe_iterate (vbases, i, &binfo); i++)

> 

> Ah, yes, thanks, I should have looked at the wider context.  The patch is

> OK.


Thanks!  I checked in the patch.

Marek
Martin Sebor June 11, 2019, 3:50 p.m. | #12
On 6/11/19 7:59 AM, Matthew Beliveau wrote:
> Hello,

> 

> Correct me if I'm wrong, but before the function checks for virtual

> bases, an if-statement checks for extra_warnings on line 6049(when my

> patch is applied). The check was there before I made my changes, and

> my test fails

> without  -Wextra.  Below is the code above the warning call:

> 

>    if (extra_warnings)

>      for (vbases = CLASSTYPE_VBASECLASSES (t), i = 0;

> vec_safe_iterate (vbases, i, &binfo); i++)

>        {

> basetype = BINFO_TYPE (binfo);


Just as an FYI, testing warning flags prevents #pragma GCC
diagnostic from having the expected effect:

   struct A { };
   struct B : virtual A { };
   struct C : A { };
   struct D0 : B, C { };   // -Winaccessible-base (good)

   #pragma GCC diagnostic push
   #pragma GCC diagnostic ignored "-Wextra"
   struct D1 : B, C { };   //  -Winaccessible-base not suppressed
   #pragma GCC diagnostic pop

   struct D2 : B, C { };   // -Winaccessible-base (good)

(A more realistic use case is supressing -Wextra before including
a third party header and then restoring it.)

This can be solved by adding a function (say, is_warning_enabled
(int)) that does the same #pragma-sensitive checking as warning()
and calling it instead of testing extra_warnings directly.  I.e.,

   if (is_diagnostic_enabled (OPT_Wextra))
     ...
     warning (OPT_Winaccessible_base, ...);

Martin

> 

> Unless I'm mistaken and you meant for me to check for extra_warnings

> again in the if-statement directly above the warning call.

> 

> Thank you,

> Matthew Beliveau

> 

> On Tue, Jun 11, 2019 at 12:43 AM Jason Merrill <jason@redhat.com> wrote:

>>

>> On 6/10/19 12:02 PM, Matthew Beliveau wrote:

>>>        if (!uniquely_derived_from_p (basetype, t))

>>> -       warning (OPT_Wextra, "virtual base %qT inaccessible in %qT due "

>>> -                "to ambiguity", basetype, t);

>>> +       warning (OPT_Winaccessible_base, "virtual base %qT inaccessible in "

>>> +                "%qT due to ambiguity", basetype, t);

>>

>> You mentioned using -Wextra for this message, and you have a testcase

>> for it, but here you remove the only connection between this message and

>> -Wextra: with this patch, the virtual base warning will be enabled by

>> default.  Perhaps you want to check extra_warnings in the if condition?

>>

>> Jason

Patch

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-06-07  Matthew Beliveau  <mbelivea@redhat.com>

       PR c++/90449 - add -Winaccessible-base option.
       * doc/invoke.texi (Winaccessible-base): Document.

       * c.opt (Winaccessible-base): Added new option.

       * class.c (warn_about_ambiguous_bases): Implemented new
       Winaccessible-base warning option for both direct and virtual
       base warnings.

       	* g++.dg/warn/Winaccessible-base-1.C: New file.
       	* g++.dg/warn/Winaccessible-base-2.C: New file.
       	* g++.dg/warn/Winaccessible-virtual-base-1.C: New file.
       	* g++.dg/warn/Winaccessible-virtual-base-2.C: New file.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 046d489f7eb..144f6da15d6 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -625,6 +625,10 @@  Wignored-attributes
 C C++ Var(warn_ignored_attributes) Init(1) Warning
 Warn whenever attributes are ignored.

+Winaccessible-base
+C++ ObjC++ Var(warn_inaccessible_base) Init(1) Warning
+Warn when a base is inaccessible in derived due to ambiguity.
+
 Wincompatible-pointer-types
 C ObjC Var(warn_incompatible_pointer_types) Init(1) Warning
 Warn when there is a conversion between pointers that have incompatible types.
diff --git gcc/cp/class.c gcc/cp/class.c
index a2585a61f96..0f26b34e38e 100644
--- gcc/cp/class.c
+++ gcc/cp/class.c
@@ -6025,6 +6025,10 @@  warn_about_ambiguous_bases (tree t)
   tree binfo;
   tree base_binfo;

+  /* If not checking for warning then return early.  */
+  if (!warn_inaccessible_base)
+    return;
+
   /* If there are no repeated bases, nothing can be ambiguous.  */
   if (!CLASSTYPE_REPEATED_BASE_P (t))
     return;
@@ -6036,8 +6040,8 @@  warn_about_ambiguous_bases (tree t)
       basetype = BINFO_TYPE (base_binfo);

       if (!uniquely_derived_from_p (basetype, t))
-	warning (0, "direct base %qT inaccessible in %qT due to ambiguity",
-		 basetype, t);
+	warning (OPT_Winaccessible_base, "direct base %qT inaccessible "
+		 "in %qT due to ambiguity", basetype, t);
     }

   /* Check for ambiguous virtual bases.  */
@@ -6048,8 +6052,8 @@  warn_about_ambiguous_bases (tree t)
 	basetype = BINFO_TYPE (binfo);

 	if (!uniquely_derived_from_p (basetype, t))
-	  warning (OPT_Wextra, "virtual base %qT inaccessible in %qT due "
-		   "to ambiguity", basetype, t);
+	  warning (OPT_Winaccessible_base, "virtual base %qT inaccessible in "
+		   "%qT due to ambiguity", basetype, t);
       }
 }

diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 3e4f012b4fa..862ee794773 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -317,6 +317,7 @@  Objective-C and Objective-C++ Dialects}.
 -Wignored-qualifiers  -Wignored-attributes  -Wincompatible-pointer-types @gol
 -Wimplicit  -Wimplicit-fallthrough  -Wimplicit-fallthrough=@var{n} @gol
 -Wimplicit-function-declaration  -Wimplicit-int @gol
+-Winaccessible-base @gol
 -Winit-self  -Winline  -Wno-int-conversion  -Wint-in-bool-context @gol
 -Wno-int-to-pointer-cast  -Winvalid-memory-model  -Wno-invalid-offsetof @gol
 -Winvalid-pch  -Wlarger-than=@var{byte-size} @gol
@@ -4800,6 +4801,22 @@  is only active when @option{-fdelete-null-pointer-checks} is active,
 which is enabled by optimizations in most targets.  The precision of
 the warnings depends on the optimization options used.

+@item -Winaccessible-base @r{(C++, Objective-C++ only)}
+@opindex Winaccessible-base
+@opindex Wno-inaccessible-base
+Warn when a base is inaccessible in derived due to ambiguity.  The warning is
+enabled by default.  Note the warning for virtual bases is enabled by the
+@option{-Wextra} option.
+@smallexample
+@group
+struct A @{ int a; @};
+
+struct B : A @{ @};
+
+struct C : B, A @{ @};
+@end group
+@end smallexample
+
 @item -Winit-self @r{(C, C++, Objective-C and Objective-C++ only)}
 @opindex Winit-self
 @opindex Wno-init-self
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C
new file mode 100644
index 00000000000..2e32b0b119f
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-base-1.C
@@ -0,0 +1,7 @@ 
+// PR c++/90449
+
+struct A { int a; };
+
+struct B : A { };
+
+struct C : B, A { }; // { dg-warning "direct base 'A' inaccessible in 'C' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C
new file mode 100644
index 00000000000..67bd740a763
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-base-2.C
@@ -0,0 +1,8 @@ 
+// PR c++/90449
+// { dg-options -Wno-inaccessible-base }
+
+struct A { int a; };
+
+struct B : A { };
+
+struct C : B, A { }; // { dg-bogus "direct base 'A' inaccessible in 'C' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C
new file mode 100644
index 00000000000..051fcc695fe
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-1.C
@@ -0,0 +1,10 @@ 
+// PR c++/90449
+// { dg-options -Wextra }
+
+struct A { };
+
+struct B : virtual A { };
+
+struct C : A { };
+
+struct D : B, C { }; // { dg-warning "virtual base 'A' inaccessible in 'D' due to ambiguity" }
diff --git gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C
new file mode 100644
index 00000000000..eab9ec0e3d7
--- /dev/null
+++ gcc/testsuite/g++.dg/warn/Winaccessible-virtual-base-2.C
@@ -0,0 +1,10 @@ 
+// PR c++/90449
+// { dg-options "-Wextra -Wno-inaccessible-base" }
+
+struct A { };
+
+struct B : virtual A { };
+
+struct C : A { };
+
+struct D : B, C { }; // { dg-bogus "virtual base 'A' inaccessible in 'D' due to ambiguity" }