Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference collapsing works wrong #50

Closed
adromanov opened this issue Jul 17, 2018 · 12 comments · Fixed by #59
Closed

Reference collapsing works wrong #50

adromanov opened this issue Jul 17, 2018 · 12 comments · Fixed by #59
Labels
bug Something isn't working

Comments

@adromanov
Copy link

adromanov commented Jul 17, 2018

For the following piece of code:

template <class T>
void foo(T && t)
{ }

struct Test {};

int main()
{
    Test test;
    foo(test);
    return 0;
}

output of an instantiation of foo is the following:

/* First instantiated from: insights.cpp:10 */
#ifdef INSIGHTS_USE_TEMPLATE
template<>
void foo<Test &>(Test && t)
{
}
#endif

Argument type should be Test & but not Test && because of reference collapsing.

@andreasfertig
Copy link
Owner

Hello,

thank for reporting that. The collapsing is sometimes a problem with the goal to get compiling code. I need to have a deep look into this case and check to not break others.

@andreasfertig andreasfertig added the bug Something isn't working label Jul 19, 2018
@adromanov
Copy link
Author

adromanov commented Jul 27, 2018

I've made some investigation on the following snippet:

template <class T>
void foo(T && t)
{ }

int main()
{
    int test = 0; 
    foo(test);
    return 0;
}

When insights processes a template instantiation it eventually calls

static const std::string GetAsCPPStyleString(const QualType& t);

Actually, at this point t->getTypeClass() is Type::LValueReference and t->isLValueReferenceType() is true.
The reason that t.getAsString(..) returns int && is how clang implements

// File is llvm/tools/clang/lib/AST/TypePrinter.cpp 
void TypePrinter::print(const Type *T, Qualifiers Quals, raw_ostream &OS, StringRef PlaceHolder);

Simplified call graph looks like:

TypePrinter::print()
├─printBefore()
│ └─printLValueReferenceBefore()
│   ├─printBefore() // <- getPointeeTypeAsWritten = SubstTemplateTypeParmType
│   │ └─printSubstTemplateTypeParmBefore()
│   │   └─printBefore() // <- getReplacementType = LValueRefeferenceType
│   │     └─printLValueReferenceBefore()
│   │       ├─printBefore() // <- getPointeeTypeAsWritten = BuiltinType                                                                                    
│   │       │ └─printBuiltinBefore()
│   │       │   └─OS << T->getName();
│   │       └─OS << "&";
│   └─OS << "&";
├─...
└─printAfter()
  └─printLValueReferenceAfter()
    └─printAfter() // <- getPointeeTypeAsWritten = SubstTemplateTypeParmType
      └─printSubstTemplateTypeParmAfter()
        └─printAfter() // <- getReplacementType = LValueRefeferenceType
          └─printLValueReferenceAfter()
            └─printAfter() // <- getPointeeTypeAsWritten = BuiltinType
              └─printBuiltinAfter()
                └─// do nothing

So, clang itself produces 2 ampersands for lvalues. Looks like a bug in clang for me, but 'm not sure.
Anyway, I guess this issue could be solved at insights level with some workaround.

P.S.: tested on llvm-6.0.1

@andreasfertig
Copy link
Owner

Hello adromanov,

thanks for nailing it down. It sure looks like a bug in clang. And I agree, it can be workaround in insights. I already have an idea, but this issue did draw my attention more to templates. Currently I'm looking into more than just this issue. There are a couple of other things which are incorrect. As soon as I have all together I will push a (temporary) fix.
Do you know how to open a bug report to clang? Because, I would prefer them to fix it.

@adromanov
Copy link
Author

adromanov commented Jul 29, 2018

Do you know how to open a bug report to clang? Because, I would prefer them to fix it.

I'll try to report in a couple of days.

andreasfertig pushed a commit that referenced this issue Jul 29, 2018
To fix #50 it is important to start ensuring that template code is valid
and does not change. Hence all compiling code does define
INSIGHTS_USE_TEMPLATE to enable compilation of the transformed template
code.
andreasfertig pushed a commit that referenced this issue Jul 29, 2018
It looks like clang does handle l-value references in the AST dump
wrong. It adds a second ampersand which makes it an r-value reference.
As #50 points out this second ampersand leads to invalid code. The
workaround implemented here does a simple (but expensive) string search
in case of an l-value ref and simply replaces the && with just &.
andreasfertig pushed a commit that referenced this issue Jul 29, 2018
Fix #50: Remove second and invalid ampersand for an l-value ref.
@adromanov
Copy link
Author

That workaround doesn't work with:

template <class F>
void foo(F && f)
{ }

using F = int && (int &&);                                                                                                                                                                                         

int && f(int && i)
{
    return static_cast<int &&>(i);
}

int main()
{
    F & rf = f;
    foo(rf);
    return 0;
}

Top level && should be replaced with &, not the first one. This is tricky when we have only string representation of a type.

@adromanov
Copy link
Author

@andreasfertig andreasfertig reopened this Aug 2, 2018
@andreasfertig
Copy link
Owner

Thanks! I'm curious what they say. I will revert the not so well working patch.

@adromanov
Copy link
Author

Please, remove double ampersand hack from static const std::string GetAsCPPStyleString(const QualType& t):

  1. it doesn't work properly
  2. With new LLVM 7.0.0 there is no need in it - everything works fine, & is printed instead of &&

Thanks!

andreasfertig pushed a commit that referenced this issue Sep 20, 2018
andreasfertig added a commit that referenced this issue Sep 20, 2018
@andreasfertig
Copy link
Owner

Hello adromanov,

I just reverted the hack. However, I could not confirm that it is fixed in my clang 7.0.0 version. That could be as it is a couple days if not weeks old. Also it will not show up in the web-frontend as Travis CI does not support the new llvm 7 repository yet. At least not easily.

@adromanov
Copy link
Author

I've built a fresh 7.0.0 yesterday (tags/RELEASE_700/final) - it works for me.

@andreasfertig
Copy link
Owner

Ok now I see it as well. The next problem is static linking such that I can use it for the web frontend.
I leave this issue open until I have figured it out.

Thanks for keeping an eye on that issue!

@andreasfertig
Copy link
Owner

Hello adromanov,

I finally updated to Clang 7.0. As far as I can see this issue is be solved now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants