Page MenuHomePhabricator

Fix crash on switch conditions of non-integer types in templates
ClosedPublic

Authored by eandrews on Apr 23 2019, 9:31 AM.

Details

Summary

Clang currently crashes for switch statements inside a template when the condition is a non-integer field member because contextual implicit conversion is skipped when parsing the condition. This conversion is however later checked in an assert when the case statement is handled.

The conversion is skipped when parsing the condition because the field member is set as type-dependent based on its containing class. This patch sets the type dependency based on the field's type instead. A few lit tests started to fail once I applied this patch because errors are now diagnosed earlier (does not wait till instantiation). I've modified these tests.

This patch fixes Bug 40982. Please note my earlier attempt at fixing this bug delayed checks for dependent expressions till instantiation. I changed the approach after feedback from Richard and cpplearner (ensadc) - https://bugs.llvm.org/show_bug.cgi?id=40982

Diff Detail

Repository
rL LLVM

Event Timeline

eandrews created this revision.Apr 23 2019, 9:31 AM

I apologize for the confusion. I was working on an incorrect branch earlier, and so abandoned that patch and uploaded a new revision.

(I am ensadc at bugzilla)

Does this cause regression in the following case?

template<int *N>
void f() {
    switch (N) case 0:; // should be diagnosed
    switch (0) case N:; // should be diagnosed
}

Currently clang diagnoses these switch statements even if the template is not instantiated, GCC does not.

AFAIK both compilers are correct under [temp.res]/8, but clang's current behavior seems more helpful.

I ran the test you provided and it does throw errors without instantiation

bash-4.2$ clang -cc1 test/SemaTemplate/test2.cpp
test/SemaTemplate/test2.cpp:3:7: error: statement requires expression of integer type ('int *' invalid)
      switch (N) case 0:; // should be diagnosed
      ^       ~
test/SemaTemplate/test2.cpp:4:23: error: value of type 'int *' is not implicitly convertible to 'int'
      switch (0) case N:; // should be diagnosed
                      ^
test/SemaTemplate/test2.cpp:4:25: warning: switch statement has empty body
      switch (0) case N:; // should be diagnosed
                        ^
test/SemaTemplate/test2.cpp:4:25: note: put the semicolon on a separate line to silence this warning
1 warning and 2 errors generated.

However, I am surprised it does since I expected it not to :) For example the lit test in this patch will not throw an error without the instantiation. I need to debug this further to understand what's happening. Thanks for taking a look!

eandrews updated this revision to Diff 203854.Jun 10 2019, 10:08 AM
eandrews edited the summary of this revision. (Show Details)
eandrews added a reviewer: rsmith.

ping

I just realized I modified the summary when I uploaded a new patch, but did not add a comment about the changes I made. Based on feedback from the bug report (https://bugs.llvm.org/show_bug.cgi?id=40982), I changed my approach for this crash. This patch changes the type dependency of the member which causes the crash.

Currently, when member expressions are created, their type dependency is set based on the class it is part of and not its own type. I don't quite understand why, and changing it broke a whole lot of lit tests. So, in this patch I only attempted to modify the type-dependency of 'members of current instantiation' to set it based on it's type as opposed to the containing classes' type.

rnk accepted this revision.Thu, Aug 1, 3:41 PM

Looks good to me. I was hoping Richard would take a look, but I reproduced the crash, and I'd rather see the fix land sooner than later. Thanks!

This revision is now accepted and ready to land.Thu, Aug 1, 3:41 PM

Thanks for taking a look Reid! I rebased the patch and had a conflict with one of Richard's patches and so ran the lit tests again and noticed there are several new failures. I am taking a look at those now.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 13, 8:54 AM

Sorry, but this change broke ClangTidy tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16398. I reverted it.

Sorry, but this change broke ClangTidy tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16398. I reverted it.

Yes I've been trying to reproduce it locally. Could you please let me know how you run clang-tidy tests? check-clang and check-all isn't showing this failure locally. I tried setting up clang tooling as described here - http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html but I don't find clang-tidy in builds

I think that you have to run check-clang-tools. I too was surprised that it was not included in check-clang.

Like @riccibruno said, check-clang-tools will run them. However, before committing a patch, please run check-all -- you never know what your patch can affect.

Like @riccibruno said, check-clang-tools will run them. However, before committing a patch, please run check-all -- you never know what your patch can affect.

And also you'll need to have the clang-tools-extra project enabled in the CMake configuration.

Thank you for letting me know. I did try reproducing the issue with check-all yesterday but was unable to. I do not think I build with 'clang-tools-extra' project enabled though. I'll take another look today.