From 5740f93ef40a1bb03fe2ff8907db0f552eeca7b9 Mon Sep 17 00:00:00 2001 From: Matthew Olsson Date: Sun, 19 May 2024 11:31:47 -0700 Subject: [PATCH] ClangPlugins: Check for strong root fields in GC allocated objects GC-allocated objects should never have JS::SafeFunction/JS::Handle fields. For now the plugin only emits warnings here, as there are many cases of this occurring in the codebase that aren't trivial to fix. It is also behind a CMake flag since it is a _very_ loud warning. --- Meta/CMake/common_options.cmake | 1 + Meta/Lagom/ClangPlugins/CMakeLists.txt | 3 + .../ClangPlugins/LibJSGCPluginAction.cpp | 159 +++++++++++------- Meta/Lagom/ClangPlugins/LibJSGCPluginAction.h | 15 +- Tests/ClangPlugins/CMakeLists.txt | 3 + ...rong_root_fields_in_gc_allocated_types.cpp | 26 +++ 6 files changed, 141 insertions(+), 66 deletions(-) create mode 100644 Tests/ClangPlugins/LibJSGCTests/strong_root_fields_in_gc_allocated_types.cpp diff --git a/Meta/CMake/common_options.cmake b/Meta/CMake/common_options.cmake index cfd575684e5..858741c793f 100644 --- a/Meta/CMake/common_options.cmake +++ b/Meta/CMake/common_options.cmake @@ -33,3 +33,4 @@ serenity_option(SERENITY_CACHE_DIR "${PROJECT_BINARY_DIR}/../caches" CACHE PATH serenity_option(ENABLE_NETWORK_DOWNLOADS ON CACHE BOOL "Allow downloads of required files. If OFF, required files must already be present in SERENITY_CACHE_DIR") serenity_option(ENABLE_CLANG_PLUGINS OFF CACHE BOOL "Enable building with the Clang plugins") +serenity_option(ENABLE_CLANG_PLUGINS_INVALID_FUNCTION_MEMBERS OFF CACHE BOOL "Enable detecting invalid function types as members of GC-allocated objects") diff --git a/Meta/Lagom/ClangPlugins/CMakeLists.txt b/Meta/Lagom/ClangPlugins/CMakeLists.txt index 73fe656d2bc..803c91ada90 100644 --- a/Meta/Lagom/ClangPlugins/CMakeLists.txt +++ b/Meta/Lagom/ClangPlugins/CMakeLists.txt @@ -19,6 +19,9 @@ function(depend_on_clang_plugin target_name plugin_name) add_dependencies(${target_name} ${plugin_name}Target) endif() target_compile_options(${target_name} INTERFACE -fplugin=$) + if (${ENABLE_CLANG_PLUGINS_INVALID_FUNCTION_MEMBERS}) + target_compile_options(${target_name} INTERFACE -fplugin-arg-libjs_gc_scanner-detect-invalid-function-members) + endif() endfunction() clang_plugin(LambdaCaptureClangPlugin SOURCES LambdaCapturePluginAction.cpp) diff --git a/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.cpp b/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.cpp index 0bb6182f86b..6abe108b409 100644 --- a/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.cpp +++ b/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.cpp @@ -57,7 +57,16 @@ std::vector get_all_qualified_types(clang::QualType const& type if (auto const* template_specialization = type->getAs()) { auto specialization_name = template_specialization->getTemplateName().getAsTemplateDecl()->getQualifiedNameAsString(); // Do not unwrap GCPtr/NonnullGCPtr/MarkedVector - if (specialization_name == "JS::GCPtr" || specialization_name == "JS::NonnullGCPtr" || specialization_name == "JS::RawGCPtr" || specialization_name == "JS::MarkedVector") { + static std::unordered_set gc_relevant_type_names { + "JS::GCPtr", + "JS::NonnullGCPtr", + "JS::RawGCPtr", + "JS::MarkedVector", + "JS::Handle", + "JS::SafeFunction", + }; + + if (gc_relevant_type_names.contains(specialization_name)) { qualified_types.push_back(type); } else { auto const template_arguments = template_specialization->template_arguments(); @@ -75,65 +84,75 @@ std::vector get_all_qualified_types(clang::QualType const& type return qualified_types; } - -struct FieldValidationResult { - bool is_valid { false }; - bool is_wrapped_in_gcptr { false }; - bool needs_visiting { false }; +enum class OuterType { + GCPtr, + RawGCPtr, + Handle, + SafeFunction, + Ptr, + Ref, }; -FieldValidationResult validate_field(clang::FieldDecl const* field_decl) +struct QualTypeGCInfo { + std::optional outer_type { {} }; + bool base_type_inherits_from_cell { false }; +}; + +std::optional validate_qualified_type(clang::QualType const& type) +{ + if (auto const* pointer_decl = type->getAs()) { + if (auto const* pointee = pointer_decl->getPointeeCXXRecordDecl()) + return QualTypeGCInfo { OuterType::Ptr, record_inherits_from_cell(*pointee) }; + } else if (auto const* reference_decl = type->getAs()) { + if (auto const* pointee = reference_decl->getPointeeCXXRecordDecl()) + return QualTypeGCInfo { OuterType::Ref, record_inherits_from_cell(*pointee) }; + } else if (auto const* specialization = type->getAs()) { + auto template_type_name = specialization->getTemplateName().getAsTemplateDecl()->getQualifiedNameAsString(); + + OuterType outer_type; + if (template_type_name == "JS::GCPtr" || template_type_name == "JS::NonnullGCPtr") { + outer_type = OuterType::GCPtr; + } else if (template_type_name == "JS::RawGCPtr") { + outer_type = OuterType::RawGCPtr; + } else if (template_type_name == "JS::Handle") { + outer_type = OuterType::Handle; + } else if (template_type_name == "JS::SafeFunction") { + return QualTypeGCInfo { OuterType::SafeFunction, false }; + } else { + return {}; + } + + auto template_args = specialization->template_arguments(); + if (template_args.size() != 1) + return {}; // Not really valid, but will produce a compilation error anyway + + auto const& type_arg = template_args[0]; + auto const* record_type = type_arg.getAsType()->getAs(); + if (!record_type) + return {}; + + auto const* record_decl = record_type->getAsCXXRecordDecl(); + if (!record_decl->hasDefinition()) + return {}; + + return QualTypeGCInfo { outer_type, record_inherits_from_cell(*record_decl) }; + } + + return {}; +} + +std::optional validate_field_qualified_type(clang::FieldDecl const* field_decl) { auto type = field_decl->getType(); if (auto const* elaborated_type = llvm::dyn_cast(type.getTypePtr())) type = elaborated_type->desugar(); - FieldValidationResult result { .is_valid = true }; - for (auto const& qualified_type : get_all_qualified_types(type)) { - if (auto const* pointer_decl = qualified_type->getAs()) { - if (auto const* pointee = pointer_decl->getPointeeCXXRecordDecl()) { - if (record_inherits_from_cell(*pointee)) { - result.is_valid = false; - result.is_wrapped_in_gcptr = false; - result.needs_visiting = true; - return result; - } - } - } else if (auto const* reference_decl = qualified_type->getAs()) { - if (auto const* pointee = reference_decl->getPointeeCXXRecordDecl()) { - if (record_inherits_from_cell(*pointee)) { - result.is_valid = false; - result.is_wrapped_in_gcptr = false; - result.needs_visiting = true; - return result; - } - } - } else if (auto const* specialization = qualified_type->getAs()) { - auto template_type_name = specialization->getTemplateName().getAsTemplateDecl()->getName(); - if (template_type_name != "GCPtr" && template_type_name != "NonnullGCPtr" && template_type_name != "RawGCPtr") - return result; - - auto const template_args = specialization->template_arguments(); - if (template_args.size() != 1) - return result; // Not really valid, but will produce a compilation error anyway - - auto const& type_arg = template_args[0]; - auto const* record_type = type_arg.getAsType()->getAs(); - if (!record_type) - return result; - - auto const* record_decl = record_type->getAsCXXRecordDecl(); - if (!record_decl->hasDefinition()) - return result; - - result.is_wrapped_in_gcptr = true; - result.is_valid = record_inherits_from_cell(*record_decl); - result.needs_visiting = template_type_name != "RawGCPtr"; - } + if (auto error = validate_qualified_type(qualified_type)) + return error; } - return result; + return {}; } bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record) @@ -151,17 +170,20 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record) auto& diag_engine = m_context.getDiagnostics(); std::vector fields_that_need_visiting; + auto record_is_cell = record_inherits_from_cell(*record); for (clang::FieldDecl const* field : record->fields()) { - auto validation_results = validate_field(field); - if (!validation_results.is_valid) { - if (validation_results.is_wrapped_in_gcptr) { - auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "Specialization type must inherit from JS::Cell"); - diag_engine.Report(field->getLocation(), diag_id); - } else { + auto validation_results = validate_field_qualified_type(field); + if (!validation_results) + continue; + + auto [outer_type, base_type_inherits_from_cell] = *validation_results; + + if (outer_type == OuterType::Ptr || outer_type == OuterType::Ref) { + if (base_type_inherits_from_cell) { auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "%0 to JS::Cell type should be wrapped in %1"); auto builder = diag_engine.Report(field->getLocation(), diag_id); - if (field->getType()->isReferenceType()) { + if (outer_type == OuterType::Ref) { builder << "reference" << "JS::NonnullGCPtr"; } else { @@ -169,12 +191,24 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record) << "JS::GCPtr"; } } - } else if (validation_results.needs_visiting) { - fields_that_need_visiting.push_back(field); + } else if (outer_type == OuterType::GCPtr || outer_type == OuterType::RawGCPtr) { + if (!base_type_inherits_from_cell) { + auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "Specialization type must inherit from JS::Cell"); + diag_engine.Report(field->getLocation(), diag_id); + } else if (outer_type == OuterType::GCPtr) { + fields_that_need_visiting.push_back(field); + } + } else if (outer_type == OuterType::Handle || outer_type == OuterType::SafeFunction) { + if (record_is_cell && m_detect_invalid_function_members) { + // FIXME: Change this to an Error when all of the use cases get addressed and remove the plugin argument + auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "Types inheriting from JS::Cell should not have %0 fields"); + auto builder = diag_engine.Report(field->getLocation(), diag_id); + builder << (outer_type == OuterType::Handle ? "JS::Handle" : "JS::SafeFunction"); + } } } - if (!record_inherits_from_cell(*record)) + if (!record_is_cell) return true; validate_record_macros(*record); @@ -406,8 +440,9 @@ void LibJSGCVisitor::validate_record_macros(clang::CXXRecordDecl const& record) report_missing_macro(); } -LibJSGCASTConsumer::LibJSGCASTConsumer(clang::CompilerInstance& compiler) +LibJSGCASTConsumer::LibJSGCASTConsumer(clang::CompilerInstance& compiler, bool detect_invalid_function_members) : m_compiler(compiler) + , m_detect_invalid_function_members(detect_invalid_function_members) { auto& preprocessor = compiler.getPreprocessor(); preprocessor.addPPCallbacks(std::make_unique(preprocessor, m_macro_map)); @@ -415,7 +450,7 @@ LibJSGCASTConsumer::LibJSGCASTConsumer(clang::CompilerInstance& compiler) void LibJSGCASTConsumer::HandleTranslationUnit(clang::ASTContext& context) { - LibJSGCVisitor visitor { context, m_macro_map }; + LibJSGCVisitor visitor { context, m_macro_map, m_detect_invalid_function_members }; visitor.TraverseDecl(context.getTranslationUnitDecl()); } diff --git a/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.h b/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.h index 405b51cc798..4841c944331 100644 --- a/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.h +++ b/Meta/Lagom/ClangPlugins/LibJSGCPluginAction.h @@ -53,9 +53,10 @@ private: class LibJSGCVisitor : public clang::RecursiveASTVisitor { public: - explicit LibJSGCVisitor(clang::ASTContext& context, LibJSCellMacroMap const& macro_map) + explicit LibJSGCVisitor(clang::ASTContext& context, LibJSCellMacroMap const& macro_map, bool detect_invalid_function_members) : m_context(context) , m_macro_map(macro_map) + , m_detect_invalid_function_members(detect_invalid_function_members) { } @@ -72,33 +73,39 @@ private: clang::ASTContext& m_context; LibJSCellMacroMap const& m_macro_map; + bool m_detect_invalid_function_members; }; class LibJSGCASTConsumer : public clang::ASTConsumer { public: - explicit LibJSGCASTConsumer(clang::CompilerInstance&); + LibJSGCASTConsumer(clang::CompilerInstance&, bool detect_invalid_function_members); private: virtual void HandleTranslationUnit(clang::ASTContext& context) override; clang::CompilerInstance& m_compiler; LibJSCellMacroMap m_macro_map; + bool m_detect_invalid_function_members; }; class LibJSGCPluginAction : public clang::PluginASTAction { public: - virtual bool ParseArgs(clang::CompilerInstance const&, std::vector const&) override + virtual bool ParseArgs(clang::CompilerInstance const&, std::vector const& args) override { + m_detect_invalid_function_members = std::find(args.begin(), args.end(), "detect-invalid-function-members") != args.end(); return true; } virtual std::unique_ptr CreateASTConsumer(clang::CompilerInstance& compiler, llvm::StringRef) override { - return std::make_unique(compiler); + return std::make_unique(compiler, m_detect_invalid_function_members); } ActionType getActionType() override { return AddAfterMainAction; } + +private: + bool m_detect_invalid_function_members { false }; }; diff --git a/Tests/ClangPlugins/CMakeLists.txt b/Tests/ClangPlugins/CMakeLists.txt index 4d1a68d11a5..88dee4b4d73 100644 --- a/Tests/ClangPlugins/CMakeLists.txt +++ b/Tests/ClangPlugins/CMakeLists.txt @@ -6,6 +6,9 @@ find_package(Python3 REQUIRED COMPONENTS Interpreter) get_property(CLANG_PLUGINS_COMPILE_OPTIONS_FOR_TESTS GLOBAL PROPERTY CLANG_PLUGINS_COMPILE_OPTIONS_FOR_TESTS) list(APPEND CLANG_PLUGINS_COMPILE_OPTIONS_FOR_TESTS -std=c++23 -Wno-user-defined-literals -Wno-literal-range) +# Ensure we always check for invalid function field types regardless of the value of ENABLE_CLANG_PLUGINS_INVALID_FUNCTION_MEMBERS +list(APPEND CLANG_PLUGINS_COMPILE_OPTIONS_FOR_TESTS -fplugin-arg-libjs_gc_scanner-detect-invalid-function-members) + get_property(CLANG_PLUGINS_INCLUDE_DIRECTORIES TARGET AK PROPERTY INCLUDE_DIRECTORIES) list(APPEND CLANG_PLUGINS_INCLUDE_DIRECTORIES ${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES}) diff --git a/Tests/ClangPlugins/LibJSGCTests/strong_root_fields_in_gc_allocated_types.cpp b/Tests/ClangPlugins/LibJSGCTests/strong_root_fields_in_gc_allocated_types.cpp new file mode 100644 index 00000000000..6888473be45 --- /dev/null +++ b/Tests/ClangPlugins/LibJSGCTests/strong_root_fields_in_gc_allocated_types.cpp @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2024, Matthew Olsson + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +// RUN: %clang++ -cc1 -verify %plugin_opts% %s 2>&1 + +#include +#include +#include + +class CellClass : JS::Cell { + JS_CELL(CellClass, JS::Cell); + + // expected-warning@+1 {{Types inheriting from JS::Cell should not have JS::SafeFunction fields}} + JS::SafeFunction m_func; + + // expected-warning@+1 {{Types inheriting from JS::Cell should not have JS::Handle fields}} + JS::Handle m_handle; +}; + +class NonCellClass { + JS::SafeFunction m_func; + JS::Handle m_handle; +};