Page MenuHomePhabricator

[Sema] Catch a case when 'volatile' qualifier is dropped while binding
AbandonedPublic

Authored by davide on Jul 1 2015, 1:34 PM.

Details

Reviewers
rjmccall
rsmith
Summary

This is an attempt to fix https://llvm.org/bugs/show_bug.cgi?id=23882
Sema doesn't diagnose that binding 'volatile W' to to reference of type 'const W&' will discard qualifier and we end up in an infinite loop later on (until we run out of stack).

InitSequence::Perform() -> ... -> GatherArgumentsForCall() -> InitSequence::Perform()

I'm not sure if the check is OK as is, or needs to be generalized and/or moved.
I'll add the testcase in the PR if this review is accepted.

Thanks,

Davide

Diff Detail

Event Timeline

davide updated this revision to Diff 28894.Jul 1 2015, 1:34 PM
davide retitled this revision from to [Sema] Catch a case when 'volatile' qualifier is dropped while binding .
davide updated this object.
davide edited the test plan for this revision. (Show Details)
davide added reviewers: rsmith, rjmccall.
davide set the repository for this revision to rL LLVM.
davide added a subscriber: Unknown Object (MLST).

Very gentle ping.

This needs a test case.

davide updated this revision to Diff 29640.Jul 13 2015, 7:52 PM
davide removed rL LLVM as the repository for this revision.

Testcase.

rjmccall edited edge metadata.Jul 13 2015, 9:09 PM

That doesn't seem like a good diagnostic. The actual problem is that you can't copy-construct a W from a volatile W&, but the error is being reported at a lower level than overload resolution; and even if you accept that as given, it's still complaining about the qualifier mismatch as if that's more important that the fact that the types don't match up.

I think this is probably just not the right place to bail out of the loop.

Also, does the bug not reproduce if the S constructor's parameter isn't meaninglessly volatile-qualified? It's possible that we're just not stripping that properly in some place.

cynecx added a subscriber: cynecx.Sep 17 2015, 3:18 PM
cynecx removed a subscriber: cynecx.
cynecx added a comment.EditedSep 17 2015, 3:52 PM

Hi :),

First of all, I would like to say that I am new into all the clang stuff (software architecture/internal concepts) but I wanted to start hacking clang so I went here to check out some trivial clang bugs to fix so I can learn internal stuff about clang, with internal stuff I mean how AST/Sema are implemented and kinda these things.

So basically, I tried all the day long to find a better approach as @rjmccall explained that the key issue is that you can't copy-construct a W from a volatile W&. The issue might be in SemaInit.cpp -> ResolveConstructorOverload because when the NamedDecl* loop reaches the const S& constructor it doesn't know that it's actually copy-constructed so it's not setting SuppressUserConversions to true. This has the effect that AddTemplateOverloadCandidate also includes user-conversion functions, so in this case: S(volatile W) and this is actually what causes the infinity loop: PerformCopyInitialization -> InitializationSequence::Perform ->CompleteConstructorCall -> GatherArgumentsForCall -> ....
While debugging I basically changed the SuppressUserConversions variable to true for the W (const S&) constructor iteration and this solved the infinity-loop problem and produced a gcc similar diagnostic message:

simplified.cpp:13:7: error: no matching constructor for initialization of 'W'
  S s(*w);
      ^~
simplified.cpp:3:8: note: candidate constructor (the implicit copy constructor) not viable: 1st argument ('volatile W')
      would lose volatile qualifier
struct W {
       ^
simplified.cpp:3:8: note: candidate constructor (the implicit move constructor) not viable: 1st argument ('volatile W')
      would lose volatile qualifier
struct W {
       ^
simplified.cpp:4:3: note: candidate constructor not viable: no known conversion from 'volatile W' to 'const S &' for
      1st argument
  W( const S& ) {}
  ^
simplified.cpp:8:15: note: passing argument to parameter here
  S(volatile W) {}

It would be nice if anyone could check if this is a right approach for fixing this issue.
Thank you.

This is outside of my expertise, but I've asked Doug Gregor to take a look.

davide abandoned this revision.Sep 9 2016, 1:44 PM