Page MenuHomePhabricator

[clang-tidy] Reworked enum options handling(again)
ClosedPublic

Authored by njames93 on Jun 19 2020, 7:25 AM.

Details

Summary

Following on from D77085, I was never happy with the passing a mapping to the option get/store functions. This patch addresses this by using explicit specializations to handle the serializing and deserializing of enum options.

Diff Detail

Event Timeline

njames93 created this revision.Jun 19 2020, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 7:25 AM
njames93 marked an inline comment as done.Jun 19 2020, 8:38 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
30

As a side bonus, this originally incorrect code would cast IncludeStyle to an int, then store that value. Now it will store it as the correct enum. Any other enums passed to store will error out if there is no mapping specialization for them, forcing you to either declare the mapping or cast it to an integer type

njames93 updated this revision to Diff 272306.Jun 21 2020, 5:23 AM

Update doc comments.

aaron.ballman accepted this revision.Jun 21 2020, 9:53 AM

I like the direction of this, thank you! LGTM with a small suggestion, but you should wait a few days in case one of the other reviewers has comments.

clang-tools-extra/clang-tidy/ClangTidyCheck.h
30–31

Any reason not to do:

static ArrayRef<std::pair<T, StringRef>> getEnumMapping() = delete;

so you get a nice error if you instantiate something without the required definition?

This revision is now accepted and ready to land.Jun 21 2020, 9:53 AM
njames93 updated this revision to Diff 272317.Jun 21 2020, 10:48 AM
njames93 marked 2 inline comments as done.

Use = delete for getEnumMapping in template definition.

clang-tools-extra/clang-tidy/ClangTidyCheck.h
30–31

I need to read the c++ standard, never even realised you could delete static functions like that.

njames93 updated this revision to Diff 272336.Jun 21 2020, 10:41 PM
  • Fix compilation failure on rebase
This revision was automatically updated to reflect the committed changes.

This breaks chevk-clang-tools in mac: http://45.33.8.238/mac/16292/step_8.txt

Please take a look and revert for now if it takes a while to fix.

This breaks chevk-clang-tools in mac: http://45.33.8.238/mac/16292/step_8.txt

Please take a look and revert for now if it takes a while to fix.

Are you sure that this is the offending patch? I don't touch the diagnostics consumer in this patch and this is essentially NFC.

It's the only change on the blame list, things pass consistently before the change, and fail consistently after it.

I reverted this in 8f73c4432b5fa8510c99a5053c07dc70a610e1fb and check-clang-tools stopped failing.

njames93 added a comment.EditedJun 29 2020, 12:41 AM

It's the only change on the blame list, things pass consistently before the change, and fail consistently after it.

Do you happen to have a proper build log for the failing build. That bot doesn't show what happened when building, in fact it says there was no work to do when building which seems to confuse me. My best guess for the fail is infrastructure related.

Edit: http://45.33.8.238/mac/16282/log.txt this is the log I want. It shows the failure and that this is the cause, but I'm still struggling to see what the actual cause is?????

I've relanded this with some sanity checks in 37cc4fa2eaa3d03ca8cd4947eb0d4c60e3c9b45c. If it fails again without those asserts I'll go back to the drawing board

njames93 reopened this revision.Jun 29 2020, 1:59 AM
This revision is now accepted and ready to land.Jun 29 2020, 1:59 AM
njames93 updated this revision to Diff 274012.Jun 29 2020, 1:59 AM

Figuring out mac crash

njames93 marked an inline comment as done.Jun 29 2020, 2:00 AM
njames93 added inline comments.
clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
31 ↗(On Diff #274012)

This assert is failing on the mac builds - http://45.33.8.238/mac/16309/step_8.txt yet I can see no logical reason for it

@thakis Do those bots use gn, could that be the cause of the failures?
From what I can see gn isn't fully supported by llvm and could certainly be the cause of failing builds.
Do we have any build bots on mac that don't use gn but passed the test case?

@thakis Do those bots use gn, could that be the cause of the failures?

It's possible, but unlikely, given that your change doesn't touch any build files.

…and indeed, I checked out 8f73c4432b5fa8510c99a5053c07dc70a610e1fb^ (the commit before the revert) and ran ninja -j200 check-clang-tools in a cmake build and it repros there too:

-- Build files have been written to: /Users/thakis/src/llvm-build-project
[2185/2306] Linking CXX static library lib/libclangDriver.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning same member name (VE.cpp.o) in output file used for input files: tools/clang/lib/Driver/CMakeFiles/obj.clangDriver.dir/ToolChains/VE.cpp.o and: tools/clang/lib/Driver/CMakeFiles/obj.clangDriver.dir/ToolChains/Arch/VE.cpp.o (due to use of basename, truncation, blank padding or duplicate input files)
[2305/2306] Running the Clang extra tools' regression tests
FAIL: Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors (799 of 924)
******************** TEST 'Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors' FAILED ********************
Note: Google Test filter = ClangTidyDiagnosticConsumer.SortsErrors
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ClangTidyDiagnosticConsumer
[ RUN      ] ClangTidyDiagnosticConsumer.SortsErrors
/Users/thakis/src/llvm-project/clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp:27: Failure
      Expected: 2ul
      Which is: 2
To be equal to: Errors.size()
      Which is: 0
0  ClangTidyTests           0x0000000106468c65 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1  ClangTidyTests           0x0000000106467b36 llvm::sys::RunSignalHandlers() + 198
2  ClangTidyTests           0x0000000106469246 SignalHandler(int) + 262
3  libsystem_platform.dylib 0x00007fff6b5055fd _sigtramp + 29
4  libsystem_malloc.dylib   0x00007fff6b4c5a3d tiny_malloc_from_free_list + 555
5  ClangTidyTests           0x000000010647186f testing::Test::Run() + 527
6  ClangTidyTests           0x00000001064726e6 testing::TestInfo::Run() + 566
7  ClangTidyTests           0x0000000106472f17 testing::TestCase::Run() + 247
8  ClangTidyTests           0x000000010647b737 testing::internal::UnitTestImpl::RunAllTests() + 1287
9  ClangTidyTests           0x000000010647b1bb testing::UnitTest::Run() + 171
10 ClangTidyTests           0x000000010646a4c9 main + 121
11 libdyld.dylib            0x00007fff6b30ccc9 start + 1

********************
********************
Failed Tests (1):
  Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors


Testing Time: 21.55s
  Unsupported      :   3
  Passed           : 918
  Expectedly Failed:   2
  Failed           :   1

I then did the same again with the change reverted, and everything passes there.

From what I can see gn isn't fully supported by llvm and could certainly be the cause of failing builds.
Do we have any build bots on mac that don't use gn but passed the test case?

It's correct that the gn build isn't supported (not just not fully: not at all) and if this would break things only there, then that wouldn't be a reason for revert. In practice, things very rarely are broken in the GN build only, and the GN bots cycle an order of magnitude faster, so they tend to show problems before the cmake bots do.

In this case, trunk was broken for > 12h, so I'd expect there's lots of evidence of this being broken on cmake bots too. However, the mac bots aren't on buildbot but on http://green.lab.llvm.org/green/ for whatever reason, and I find that page hard to read. http://green.lab.llvm.org/green/job/clang-stage1-RA/ has a bunch of red on the LHS … ah yeah http://green.lab.llvm.org/green/job/clang-stage1-RA/11966/console has " Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors" at the bottom.

So this showed up on the regular mac bots, yes.


The tree's been broken for over half a day to this. I'd suggest taking breakage supports a bit more serious going forward, instead of replying with "it can't possibly be this patch" and "it must be bot setup weirdness" going forward :)

In this case, trunk was broken for > 12h, so I'd expect there's lots of evidence of this being broken on cmake bots too. However, the mac bots aren't on buildbot but on http://green.lab.llvm.org/green/ for whatever reason, and I find that page hard to read. http://green.lab.llvm.org/green/job/clang-stage1-RA/ has a bunch of red on the LHS … ah yeah http://green.lab.llvm.org/green/job/clang-stage1-RA/11966/console has " Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors" at the bottom.

So this showed up on the regular mac bots, yes.


The tree's been broken for over half a day to this. I'd suggest taking breakage supports a bit more serious going forward, instead of replying with "it can't possibly be this patch" and "it must be bot setup weirdness" going forward :)

Thanks for clarifying about the regular mac bots. Is this isolated to just mac bots?

If it is isolated then the logical reason for this failure would be a subtle bug in the host toolchain.
When I landed for a second time, I noticed that the TestCheck in ClangTidyDiagnosticsConsumerTest.cpp gets instantiated, but the registerMatchers function never gets called, this is the root cause for why the test fails.
However you just need to read the the code to see how that situation shouldn't happen.
Without a mac to test on, I wont be able to investigate further(no pre merge mac test bot also makes this significantly harder for me to test), Tried testing on older clang compiler and wasn't able to reproduce any error.

njames93 updated this revision to Diff 275520.Jul 4 2020, 2:08 PM

Solved the mac issue

njames93 marked an inline comment as done.Jul 4 2020, 2:22 PM
njames93 added inline comments.
clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
225

@aaron.ballman @thakis I figured out a way to prevent the test cases failing on mac, still can't figure out the root cause. Would this be acceptable for now?

As far as I can see some subtle linker bug in the default mac os linker isn't happy when this test case is included. This actual test case runs just fine, but the test in ClangTidyDiagnosticsConsumerTest doesn't run properly. It appears to instantiate the context and then instantiate the check, but it never attempts to register it, causing the whole test case to fail.
clang-tidy builds just fine which has many uses of this new enum handling and check-clang-tools also runs without a hitch.

This revision was automatically updated to reflect the committed changes.