This is an archive of the discontinued LLVM Phabricator instance.

Fix for bug 17427 - Assertion failed: "Computed __func__ length differs from type!"
ClosedPublic

Authored by ABataev on Sep 16 2014, 2:52 AM.

Details

Summary

Reworked PredefinedExpr representation with internal StringLiteral field for function declaration.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 13740.Sep 16 2014, 2:52 AM
ABataev retitled this revision from to Fix for bug 17427 - Assertion failed: "Computed __func__ length differs from type!".
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added reviewers: rsmith, hfinkel.
ABataev added a subscriber: Unknown Object (MLST).
rsmith added inline comments.Sep 16 2014, 10:07 AM
lib/AST/Expr.cpp
604 ↗(On Diff #13740)

This existing code is already trying to handle this situation. Did your tests fail without your change? If so, why?

(The existing code is wrong for decltype(auto), auto*, and so on, but that is not the case your tests seem to be covering.)

rnk added a subscriber: rnk.Sep 16 2014, 11:03 AM

Coincidentally, I looked at this code the other day. IMO running the mangler twice for FUNCDNAME is silly. Is there a problem with computing the string in Sema and storing it on the PredefinedExpr? It looks like we always have to do this just to build the type.

rsmith added inline comments.Sep 16 2014, 11:48 AM
lib/AST/Expr.cpp
501 ↗(On Diff #13740)

The best fix (for the immediate issue) would probably be to use the declared type rather than the actual type: use Decl->getTypeSourceInfo()->getType() rather than Decl->getType() here.

As Reid points out, we may want to just stash the string into the PredefinedExpr object when we create it, rather than recomputing it each time.

606 ↗(On Diff #13740)

Remove the existing (broken) handling for this case.

test/CodeGenCXX/predefined-expr-cxx14.cpp
3–4 ↗(On Diff #13740)

This test does not seem quite right:

  1. Testing all the different kinds of function declarations here is gaining you zero test coverage. The thing that's relevant here is how we handle the return type, and that does not change between these cases.
  2. You're missing test coverage for the cases you fixed: decltype(auto) as a return type, and types that contain auto (but are not *exactly* auto).

Richard, yes, it fails on lines 79, 76, 88

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

16.09.2014 21:07, Richard Smith пишет:

Comment at: lib/AST/Expr.cpp:604
@@ -603,2 +603,3 @@
+ Policy.KeepAutoType = true;

if ((isa<CXXMethodDecl>(FD) &&
     cast<CXXMethodDecl>(FD)->getParent()->isLambda()) ||

This existing code is already trying to handle this situation. Did your tests fail without your change? If so, why?

(The existing code is wrong for decltype(auto), auto*, and so on, but that is not the case your tests seem to be covering.)

http://reviews.llvm.org/D5365

Reid, I though about it too. I also think it is a better solution. I'll
rework the patch.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

16.09.2014 22:03, Reid Kleckner пишет:

Coincidentally, I looked at this code the other day. IMO running the mangler twice for FUNCDNAME is silly. Is there a problem with computing the string in Sema and storing it on the PredefinedExpr? It looks like we always have to do this just to build the type.

http://reviews.llvm.org/D5365

Comment at: lib/AST/Expr.cpp:501
@@ -500,3 +500,3 @@

  Decl = Pattern;
const FunctionType *AFT = Decl->getType()->getAs<FunctionType>();
const FunctionProtoType *FT = nullptr;

The best fix (for the immediate issue) would probably be to use the declared type rather than the actual type: use Decl->getTypeSourceInfo()->getType() rather than Decl->getType() here.

As Reid points out, we may want to just stash the string into the PredefinedExpr object when we create it, rather than recomputing it each time.

Richard, I'll rework the patch and store string in PredefinedExpr.

Comment at: test/CodeGenCXX/predefined-expr-cxx14.cpp:3-4
@@ +2,4 @@
+
+ CHECK-DAG: private unnamed_addr constant [15 x i8] c"externFunction\00"
+
CHECK-DAG: private unnamed_addr constant [26 x i8] c"auto NS::externFunction()\00"

+// CHECK-DAG: private unnamed_addr constant [49 x i8] c"auto functionTemplateExplicitSpecialization(int)\00"

This test does not seem quite right:

  1. Testing all the different kinds of function declarations here is gaining you zero test coverage. The thing that's relevant here is how we handle the return type, and that does not change between these cases.
  2. You're missing test coverage for the cases you fixed: decltype(auto) as a return type, and types that contain auto (but are not *exactly* auto).

http://reviews.llvm.org/D5365

The test is too big, but it has test cases for , I'll reduce it.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

16.09.2014 22:48, Richard Smith пишет:

Comment at: lib/AST/Expr.cpp:501
@@ -500,3 +500,3 @@

  Decl = Pattern;
const FunctionType *AFT = Decl->getType()->getAs<FunctionType>();
const FunctionProtoType *FT = nullptr;

The best fix (for the immediate issue) would probably be to use the declared type rather than the actual type: use Decl->getTypeSourceInfo()->getType() rather than Decl->getType() here.

As Reid points out, we may want to just stash the string into the PredefinedExpr object when we create it, rather than recomputing it each time.

Comment at: lib/AST/Expr.cpp:606
@@ -604,3 +605,3 @@

   cast<CXXMethodDecl>(FD)->getParent()->isLambda()) ||
  (FT && FT->getReturnType()->getAs<AutoType>()))
Proto = "auto " + Proto;

Remove the existing (broken) handling for this case.

Comment at: test/CodeGenCXX/predefined-expr-cxx14.cpp:3-4
@@ +2,4 @@
+
+ CHECK-DAG: private unnamed_addr constant [15 x i8] c"externFunction\00"
+
CHECK-DAG: private unnamed_addr constant [26 x i8] c"auto NS::externFunction()\00"

+// CHECK-DAG: private unnamed_addr constant [49 x i8] c"auto functionTemplateExplicitSpecialization(int)\00"

This test does not seem quite right:

  1. Testing all the different kinds of function declarations here is gaining you zero test coverage. The thing that's relevant here is how we handle the return type, and that does not change between these cases.
  2. You're missing test coverage for the cases you fixed: decltype(auto) as a return type, and types that contain auto (but are not *exactly* auto).

http://reviews.llvm.org/D5365

ABataev updated this revision to Diff 13780.Sep 17 2014, 5:43 AM
ABataev updated this object.
rsmith added inline comments.Sep 25 2014, 4:23 PM
include/clang/AST/Expr.h
1164 ↗(On Diff #13780)

Please don't move this class to a different place in the file (and likewise in Expr.cpp); it creates history churn and makes the patch hard to review. Instead, you can move the getStringLiteral method definitions out of the class (either to after the definition of StringLiteral or into the .cpp file).

1577 ↗(On Diff #13780)

I don't think you need to pass in T here; it can be obtained from the StringLiteral.

1598 ↗(On Diff #13780)

Generally, we prefer to make the ASTReader a friend of the class being deserialized rather than adding setters. Please do that here (you should also be able to remove the other setters in the process).

lib/AST/ExprConstant.cpp
2654 ↗(On Diff #13780)

C++ guarantees that __func__ has the same address for all evaluations within the same function. It looks like this patch will make them all be treated as distinct objects.

lib/AST/StmtProfile.cpp
472 ↗(On Diff #13780)

This doesn't seem necessary.

lib/CodeGen/CGExpr.cpp
2057 ↗(On Diff #13780)

I think we need to emit this with a specific mangled name in order to merge the func objects across translation units if it's used in an inline function.

lib/Sema/TreeTransform.h
6967 ↗(On Diff #13780)

This is wrong: unless this is overridden by the derived class, it would just call itself. Instead you should add a RebuildPredefinedExpr function, call it from here, and make it call into Sema to rebuild the PredefinedExpr.

test/CodeGenCXX/funcsig.cpp
11 ↗(On Diff #13780)

If this is supposed to be linkonce_odr, you should also check its mangled name.

rsmith edited edge metadata.Sep 25 2014, 4:40 PM

FYI: I'm investigating how to deal with the constraint that __func__ is the same object throughout a function; it's not supported by current ABIs, and it's not even clear that it's supportable without *extreme* heroics. On that basis, I think it's fine to continue treating __func__ like a string literal for now (that is, two different __func__s can evaluate to different arrays).

Richard, thanks for the review!

include/clang/AST/Expr.h
1164 ↗(On Diff #13780)

Ok

1577 ↗(On Diff #13780)

Yes, I thought about it too. But decided to make the constructor similar to all others.

1598 ↗(On Diff #13780)

Agree, thanks.

lib/AST/StmtProfile.cpp
472 ↗(On Diff #13780)

Ok

lib/CodeGen/CGExpr.cpp
2057 ↗(On Diff #13780)

Agree, I'll try to improve it.

lib/Sema/TreeTransform.h
6967 ↗(On Diff #13780)

Ok, I'll fix it

test/CodeGenCXX/funcsig.cpp
11 ↗(On Diff #13780)

Ok

ABataev updated this revision to Diff 14154.Sep 29 2014, 2:58 AM
ABataev edited edge metadata.

Reworked representation of PredefinedExpr after review.

rsmith added inline comments.Sep 29 2014, 5:13 PM
include/clang/AST/Expr.h
1184–1185 ↗(On Diff #14154)

You can delete these and directly set the members from ASTStmtReader.

lib/AST/Expr.cpp
468 ↗(On Diff #14154)

You need an llvm_unreachable here to shut up GCC's -Wreturn-type.

lib/Sema/SemaExpr.cpp
2969–2973 ↗(On Diff #14154)

Please remove the static local variable for now; we're discussing this on the standard reflectors and the requirement for this may soon be removed. The above doesn't completely fix the ABI problem anyway (if two compilers did this and came up with different strings, they could give __func__ different types). Sorry for the change in direction here.

Richard, thanks for the review!

include/clang/AST/Expr.h
1184–1185 ↗(On Diff #14154)

Ok

lib/AST/Expr.cpp
468 ↗(On Diff #14154)

Ok, I'll break in case for PrettyFunctionNoVirtual and llvm_unreachable at the end of the method.

lib/Sema/SemaExpr.cpp
2969–2973 ↗(On Diff #14154)

Ok, I'll return back to StringLiteral then.

ABataev updated this revision to Diff 14221.Sep 30 2014, 5:59 AM
rnk accepted this revision.Oct 7 2014, 4:57 PM
rnk added a reviewer: rnk.

Thanks for the cleanup! Looks good with a fix for the func-inside-block-inside-ctor test case.

lib/AST/Expr.cpp
521 ↗(On Diff #14221)

I believe this will assert in this code:

struct A {
  const char *s;
  A() {
    const char *(^b)() = ^() {
      return __func__;
    };
    s = b();
};
A a;

You probably need to handle the ctor / dtor cases separately, as is done in CodeGenModule::getBlockMangledName. Using Ctor_Base and Dtor_Base should be correct. I don't think we should ever emit two different block implementations for the base ctor and complete ctor. =/

lib/CodeGen/CGExpr.cpp
2061 ↗(On Diff #14221)

I think this can be an array of StringRefs to avoid building two tmp strings.

lib/CodeGen/CodeGenModule.cpp
2751–2752 ↗(On Diff #14221)

This change will cause duplicate string literals to not be merged across TUs on Windows. Just mangle the string literal if the ABI says we should.

test/CodeGenCXX/ms_wide_predefined_expr.cpp
3 ↗(On Diff #14221)

This test in particular should not change.

This revision is now accepted and ready to land.Oct 7 2014, 4:57 PM

Hi Reid, Thanks for the review!

lib/AST/Expr.cpp
521 ↗(On Diff #14221)

I'll add this test and fix the code to properly handle it.

lib/CodeGen/CGExpr.cpp
2061 ↗(On Diff #14221)

Agree

lib/CodeGen/CodeGenModule.cpp
2751–2752 ↗(On Diff #14221)

Agree, I'll remove it

test/CodeGenCXX/ms_wide_predefined_expr.cpp
3 ↗(On Diff #14221)

Agree, will revert it back

ABataev closed this revision.Oct 9 2014, 1:55 AM
ABataev updated this revision to Diff 14635.

Closed by commit rL219393 (authored by @ABataev).