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

Fix clang tidy error and crash #42564

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Fix clang tidy error and crash #42564

merged 1 commit into from
Jun 5, 2023

Conversation

knopp
Copy link
Member

@knopp knopp commented Jun 5, 2023

Clang tidy checks for #42399 are failing for formats_mtl.mm and context_mtl.mm. Weirdly this happens even though none of these files where modified in the branch.

For context_mtl.mm the fix is straightforward, just a missing std::move.
For formats_mtl.mm clang-tidy actually crashes on the assignments to des.depthCompareFunction and des.depth_write_enabled. The crash doesn't happen if the assignment is done through a temporary variable.

I'm not sure if we should have a workaround for the crash or disable clang-tidy on the file completely (FLUTTER_NOLINT).

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@knopp knopp force-pushed the fix_clang_tidy branch from d44141d to 5e8c290 Compare June 5, 2023 15:55
@knopp knopp requested a review from zanderso June 5, 2023 16:11
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hold on this until we see whether the rebase on #42399 helps?

@zanderso
Copy link
Member

zanderso commented Jun 5, 2023

As for the crasher in clang-tidy, @knopp if you can post more details here or in a new GitHub issue, I can try to raise visibility on that with a toolchain team.

@knopp
Copy link
Member Author

knopp commented Jun 5, 2023

I don't have much regarding the crash. I can reproduce it locally on main

> buildtools/mac-arm64/clang/bin/clang-tidy flutter/impeller/renderer/backend/metal/formats_mtl.mm
[1]    60385 segmentation fault  buildtools/mac-arm64/clang/bin/clang-tidy 
Stacktrace
  * frame #0: 0x000000010104946a clang-tidy`clang::dataflow::(anonymous namespace)::TransferVisitor::VisitMemberExpr(clang::MemberExpr const*) + 458
    frame #1: 0x000000010104c392 clang-tidy`clang::dataflow::transferCFGBlock(clang::CFGBlock const&, clang::dataflow::AnalysisContext&, std::__2::function<void (clang::CFGElement const&, clang::dataflow::TypeErasedDataflowAnalysisState const&)>) + 8498
    frame #2: 0x000000010104d50d clang-tidy`clang::dataflow::runTypeErasedDataflowAnalysis(clang::dataflow::ControlFlowContext const&, clang::dataflow::TypeErasedDataflowAnalysis&, clang::dataflow::Environment const&, std::__2::function<void (clang::CFGElement const&, clang::dataflow::TypeErasedDataflowAnalysisState const&)>) + 1373
    frame #3: 0x0000000100af7e12 clang-tidy`clang::tidy::bugprone::UncheckedOptionalAccessCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) + 9938
    frame #4: 0x0000000100720df0 clang-tidy`clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) + 352
    frame #5: 0x00000001006e4cbe clang-tidy`clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) + 206
    frame #6: 0x00000001006ed45c clang-tidy`clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchWithFilter(clang::DynTypedNode const&) + 2028
    frame #7: 0x00000001006ebf10 clang-tidy`clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) + 208
    frame #8: 0x00000001006efd0b clang-tidy`clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseNamespaceDecl(clang::NamespaceDecl*) + 155
    frame #9: 0x00000001006ec16a clang-tidy`clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) + 810
    frame #10: 0x00000001006f915b clang-tidy`clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseTranslationUnitDecl(clang::TranslationUnitDecl*) + 299
    frame #11: 0x00000001006ec7b8 clang-tidy`clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) + 2424
    frame #12: 0x00000001006ebabd clang-tidy`clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) + 749
    frame #13: 0x00000001017b681c clang-tidy`clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) + 44
    frame #14: 0x00000001019fed7a clang-tidy`clang::ParseAST(clang::Sema&, bool, bool) + 13994
    frame #15: 0x00000001017a1985 clang-tidy`clang::FrontendAction::Execute() + 85
    frame #16: 0x00000001016ee894 clang-tidy`clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 1124
    frame #17: 0x0000000100180e26 clang-tidy`clang::tooling::FrontendActionFactory::runInvocation(std::__2::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::__2::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) + 294
    frame #18: 0x000000010076f282 clang-tidy`clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> > >, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, bool, llvm::StringRef)::ActionFactory::runInvocation(std::__2::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::__2::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) + 66
    frame #19: 0x000000010076b5d0 clang-tidy`clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> > >, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, bool, llvm::StringRef) + 14400
    frame #20: 0x0000000100901773 clang-tidy`clang::tidy::clangTidyMain(int, char const**) + 27731
    frame #21: 0x0000000204120310 dyld`start + 2432

By experimenting a bit I found out that the crash doesn't happen when the assignment to des.depthCompareFunction and des.depthWriteEnabled goes through temporary variable.

@knopp
Copy link
Member Author

knopp commented Jun 5, 2023

Btw., I think it also crashes my clangd as well, so it's not just flutter buildtools. Probably should be reported to LLVM.

@@ -64,8 +64,13 @@

auto des = [[MTLDepthStencilDescriptor alloc] init];

des.depthCompareFunction = ToMTLCompareFunction(depth->depth_compare);
des.depthWriteEnabled = depth->depth_write_enabled;
// These temporary variables are necessary for clang-tidy (Fuchsia LLVM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the crash that this fixes happening on CI. Could we maybe split this out into a separate issue/PR?

Copy link
Member Author

@knopp knopp Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8779203752399309425/+/u/test:_lint_host_debug/stdout?format=raw

Screenshot 2023-06-05 at 19 12 58

The first error is a crash (no error message). This is from #42399

I can try to split the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, sorry. I see it now.

@knopp knopp merged commit 7f12e34 into flutter:main Jun 5, 2023
@knopp knopp deleted the fix_clang_tidy branch June 5, 2023 17:37
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 5, 2023
…128271)

flutter/engine@f9f7238...7f12e34

2023-06-05 matej.knopp@gmail.com Fix clang tidy error and crash (flutter/engine#42564)
2023-06-05 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from gp1k1agqxQIz0oTXV... to vAwdggqMrL1yoH_Zn... (flutter/engine#42570)
2023-06-05 zanderso@users.noreply.github.com Revert Dart SDK to 3.1.0-163.0.dev for branch alignment (flutter/engine#42569)
2023-06-05 zanderso@users.noreply.github.com Check more optional accesses (flutter/engine#42528)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from gp1k1agqxQIz to vAwdggqMrL1y

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants