diff mbox series

[v2] Fix failure building LLVM with location wrapper nodes (PR c++/83799)

Message ID 1515778644-36057-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series [v2] Fix failure building LLVM with location wrapper nodes (PR c++/83799) | expand

Commit Message

David Malcolm Jan. 12, 2018, 5:37 p.m. UTC 
On Fri, 2018-01-12 at 09:07 +0100, Markus Trippelsdorf wrote:
> On 2018.01.11 at 18:21 -0500, David Malcolm wrote:
> > diff --git a/gcc/testsuite/g++.dg/wrappers/pr83799.C
> > b/gcc/testsuite/g++.dg/wrappers/pr83799.C
> > new file mode 100644
> > index 0000000..f93c0ae
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/wrappers/pr83799.C
> > @@ -0,0 +1,18 @@
> > +class DataLayout;
> > +class TargetLoweringBase {
> > +  void getTypeLegalizationCost(const DataLayout &DL) const;
> > +};
> > +class TargetTransformInfoImplBase {
> > +  const DataLayout &DL; // this line isn't actually needed to
> > reproduce the issue
> > +};
> > +template <typename T>
> > +class TargetTransformInfoImplCRTPBase : public
> > TargetTransformInfoImplBase {};
> > +template <typename T>
> > +class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T>
> > {
> > +  const TargetLoweringBase *getTLI() const;
> > +  using TargetTransformInfoImplBase::DL;
> > +  void getArithmeticInstrCost() {
> > +    const TargetLoweringBase *TLI = getTLI();
> > +    TLI->getTypeLegalizationCost(DL);
> > +  }
> > +};
> 
> Thanks for the fix. Minor nit:
> Please make TargetLoweringBase and TargetLoweringBase a struct
> instead
> of a class, to prevent illegal access of private members.

Here's an updated version of the patch which does so (so that the
testcase compiles cleanly on clang).

I also added some more assertions to
selftest::test_type_dependent_expression_p.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
Manually tested with "make s-selftest-c++" (since we don't run the
selftests for cc1plus by default).

OK for trunk?


Blurb from v1:

PR c++/83799 reports a failure building LLVM due to a bogus
"no matching function for call to" error at a callsite like this:
  TLI->getTypeLegalizationCost(DL);
where "DL" is from:
  using TargetTransformInfoImplBase::DL;

The root cause is that type_dependent_expression_p on a USING_DECL
should return true when processing a template, but after r256448 the
the argument at the callsite is a location wrapper around the USING_DECL,
and type_dependent_expression_p erroneously returns false for it, as
it is comparing tree codes, and failing a match, then looking at types.

This prevents cp_parser_postfix_expression from using the
"build_min_nt_call_vec" path for handling the call, instead erroneously
handling it via build_new_method_call (which fails for this case).

This patch fixes the problem by stripping any location wrappers before
the various tree code tests in type_dependent_expression_p.   It fixes
the reduced test case, and the full BasicTargetTransformInfo.ii; after
this patch, the assembly generated for that latter case is identical to
that generated before r256448.

gcc/cp/ChangeLog:
	PR c++/83799
	* pt.c (type_dependent_expression_p): Strip any location wrapper
	before testing tree codes.
	(selftest::test_type_dependent_expression_p): New function.
	(selftest::cp_pt_c_tests): Call it.

gcc/testsuite/ChangeLog:
	PR c++/83799
	* g++.dg/wrappers/pr83799.C: New test case.
---
 gcc/cp/pt.c                             | 38 +++++++++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/wrappers/pr83799.C | 18 ++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr83799.C

Comments

David Malcolm Jan. 17, 2018, 3:58 p.m. UTC | #1 
On Fri, 2018-01-12 at 12:37 -0500, David Malcolm wrote:
> On Fri, 2018-01-12 at 09:07 +0100, Markus Trippelsdorf wrote:
> > On 2018.01.11 at 18:21 -0500, David Malcolm wrote:

[...snip...]

> > Thanks for the fix. Minor nit:
> > Please make TargetLoweringBase and TargetLoweringBase a struct
> > instead
> > of a class, to prevent illegal access of private members.
> 
> Here's an updated version of the patch which does so (so that the
> testcase compiles cleanly on clang).
> 
> I also added some more assertions to
> selftest::test_type_dependent_expression_p.
> 
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> Manually tested with "make s-selftest-c++" (since we don't run the
> selftests for cc1plus by default).
> 
> OK for trunk?

Jason approved the v2 patch on IRC, so I've committed it to trunk as
r256796.

[...snip...]
diff mbox series

Patch

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 4f8086b..1f868d0 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -24219,6 +24219,8 @@  type_dependent_expression_p (tree expression)
   if (expression == NULL_TREE || expression == error_mark_node)
     return false;
 
+  STRIP_ANY_LOCATION_WRAPPER (expression);
+
   /* An unresolved name is always dependent.  */
   if (identifier_p (expression)
       || TREE_CODE (expression) == USING_DECL
@@ -26657,12 +26659,48 @@  test_build_non_dependent_expr ()
 	     build_non_dependent_expr (wrapped_string_lit));
 }
 
+/* Verify that type_dependent_expression_p () works correctly, even
+   in the presence of location wrapper nodes.  */
+
+static void
+test_type_dependent_expression_p ()
+{
+  location_t loc = BUILTINS_LOCATION;
+
+  tree name = get_identifier ("foo");
+
+  /* If no templates are involved, nothing is type-dependent.  */
+  gcc_assert (!processing_template_decl);
+  ASSERT_FALSE (type_dependent_expression_p (name));
+
+  ++processing_template_decl;
+
+  /* Within a template, an unresolved name is always type-dependent.  */
+  ASSERT_TRUE (type_dependent_expression_p (name));
+
+  /* Ensure it copes with NULL_TREE and errors.  */
+  ASSERT_FALSE (type_dependent_expression_p (NULL_TREE));
+  ASSERT_FALSE (type_dependent_expression_p (error_mark_node));
+
+  /* A USING_DECL in a template should be type-dependent, even if wrapped
+     with a location wrapper (PR c++/83799).  */
+  tree using_decl = build_lang_decl (USING_DECL, name, NULL_TREE);
+  TREE_TYPE (using_decl) = integer_type_node;
+  ASSERT_TRUE (type_dependent_expression_p (using_decl));
+  tree wrapped_using_decl = maybe_wrap_with_location (using_decl, loc);
+  ASSERT_TRUE (location_wrapper_p (wrapped_using_decl));
+  ASSERT_TRUE (type_dependent_expression_p (wrapped_using_decl));
+
+  --processing_template_decl;
+}
+
 /* Run all of the selftests within this file.  */
 
 void
 cp_pt_c_tests ()
 {
   test_build_non_dependent_expr ();
+  test_type_dependent_expression_p ();
 }
 
 } // namespace selftest
diff --git a/gcc/testsuite/g++.dg/wrappers/pr83799.C b/gcc/testsuite/g++.dg/wrappers/pr83799.C
new file mode 100644
index 0000000..b4f5a69
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr83799.C
@@ -0,0 +1,18 @@ 
+struct DataLayout;
+struct TargetLoweringBase {
+  void getTypeLegalizationCost(const DataLayout &DL) const;
+};
+struct TargetTransformInfoImplBase {
+  const DataLayout &DL;
+};
+template <typename T>
+struct TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {};
+template <typename T>
+struct BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> {
+  const TargetLoweringBase *getTLI() const;
+  using TargetTransformInfoImplBase::DL;
+  void getArithmeticInstrCost() {
+    const TargetLoweringBase *TLI = getTLI();
+    TLI->getTypeLegalizationCost(DL);
+  }
+};