From 0130c2bcebc963b859d0023c46d64b5548e4707f Mon Sep 17 00:00:00 2001 From: Kamil Rakoczy Date: Fri, 3 Mar 2023 00:37:07 +0100 Subject: [PATCH] Internals: update clang attributes check report conditions (#3997) --- include/verilated.cpp | 6 +- include/verilated.h | 2 +- include/verilated_sym_props.h | 30 ++--- nodist/clang_check_attributes | 138 ++++++++++++++--------- test_regress/t/t_a5_attributes_src.pl | 2 +- test_regress/t/t_dist_attributes_bad.h | 110 ++++++++++++++++++ test_regress/t/t_dist_attributes_bad.out | 30 ++++- 7 files changed, 245 insertions(+), 73 deletions(-) diff --git a/include/verilated.cpp b/include/verilated.cpp index 5ccc66e7b..995656840 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -2982,7 +2982,7 @@ VerilatedModule::~VerilatedModule() { // VerilatedVar:: Methods // cppcheck-suppress unusedFunction // Used by applications -uint32_t VerilatedVarProps::entSize() const { +uint32_t VerilatedVarProps::entSize() const VL_MT_SAFE { uint32_t size = 1; switch (vltype()) { case VLVT_PTR: size = sizeof(void*); break; @@ -3002,7 +3002,7 @@ size_t VerilatedVarProps::totalSize() const { return size; } -void* VerilatedVarProps::datapAdjustIndex(void* datap, int dim, int indx) const { +void* VerilatedVarProps::datapAdjustIndex(void* datap, int dim, int indx) const VL_MT_SAFE { if (VL_UNLIKELY(dim <= 0 || dim > udims())) return nullptr; if (VL_UNLIKELY(indx < low(dim) || indx > high(dim))) return nullptr; const int indxAdj = indx - low(dim); @@ -3121,7 +3121,7 @@ void* VerilatedScope::exportFindNullError(int funcnum) VL_MT_SAFE { return nullptr; } -void* VerilatedScope::exportFindError(int funcnum) const { +void* VerilatedScope::exportFindError(int funcnum) const VL_MT_SAFE { // Slowpath - Called only when find has failed const std::string msg = (std::string{"Testbench C called '"} + VerilatedImp::exportName(funcnum) diff --git a/include/verilated.h b/include/verilated.h index db8cf9f63..822fc89fa 100644 --- a/include/verilated.h +++ b/include/verilated.h @@ -647,7 +647,7 @@ public: // But internals only - called from VerilatedModule's VerilatedVar* varFind(const char* namep) const VL_MT_SAFE_POSTINIT; VerilatedVarNameMap* varsp() const VL_MT_SAFE_POSTINIT { return m_varsp; } void scopeDump() const; - void* exportFindError(int funcnum) const; + void* exportFindError(int funcnum) const VL_MT_SAFE; static void* exportFindNullError(int funcnum) VL_MT_SAFE; static void* exportFind(const VerilatedScope* scopep, int funcnum) VL_MT_SAFE { if (VL_UNLIKELY(!scopep)) return exportFindNullError(funcnum); diff --git a/include/verilated_sym_props.h b/include/verilated_sym_props.h index f3167126c..bcf9c01a5 100644 --- a/include/verilated_sym_props.h +++ b/include/verilated_sym_props.h @@ -137,35 +137,35 @@ public: ~VerilatedVarProps() = default; // METHODS bool magicOk() const { return m_magic == MAGIC; } - VerilatedVarType vltype() const { return m_vltype; } + VerilatedVarType vltype() const VL_MT_SAFE { return m_vltype; } VerilatedVarFlags vldir() const { return static_cast(static_cast(m_vlflags) & VLVF_MASK_DIR); } - uint32_t entSize() const; + uint32_t entSize() const VL_MT_SAFE; bool isPublicRW() const { return ((m_vlflags & VLVF_PUB_RW) != 0); } // DPI compatible C standard layout bool isDpiCLayout() const { return ((m_vlflags & VLVF_DPI_CLAY) != 0); } - int udims() const { return m_udims; } + int udims() const VL_MT_SAFE { return m_udims; } int dims() const { return m_pdims + m_udims; } - const VerilatedRange& packed() const { return m_packed; } + const VerilatedRange& packed() const VL_MT_SAFE { return m_packed; } const VerilatedRange& unpacked() const { return m_unpacked[0]; } // DPI accessors - int left(int dim) const { + int left(int dim) const VL_MT_SAFE { return dim == 0 ? m_packed.left() : VL_LIKELY(dim >= 1 && dim <= udims()) ? m_unpacked[dim - 1].left() : 0; } - int right(int dim) const { + int right(int dim) const VL_MT_SAFE { return dim == 0 ? m_packed.right() : VL_LIKELY(dim >= 1 && dim <= udims()) ? m_unpacked[dim - 1].right() : 0; } - int low(int dim) const { + int low(int dim) const VL_MT_SAFE { return dim == 0 ? m_packed.low() : VL_LIKELY(dim >= 1 && dim <= udims()) ? m_unpacked[dim - 1].low() : 0; } - int high(int dim) const { + int high(int dim) const VL_MT_SAFE { return dim == 0 ? m_packed.high() : VL_LIKELY(dim >= 1 && dim <= udims()) ? m_unpacked[dim - 1].high() : 0; @@ -175,7 +175,7 @@ public: : VL_LIKELY(dim >= 1 && dim <= udims()) ? m_unpacked[dim - 1].increment() : 0; } - int elements(int dim) const { + int elements(int dim) const VL_MT_SAFE { return dim == 0 ? m_packed.elements() : VL_LIKELY(dim >= 1 && dim <= udims()) ? m_unpacked[dim - 1].elements() : 0; @@ -183,7 +183,7 @@ public: // Total size in bytes (note DPI limited to 4GB) size_t totalSize() const; // Adjust a data pointer to access a given array element, NULL if something goes bad - void* datapAdjustIndex(void* datap, int dim, int indx) const; + void* datapAdjustIndex(void* datap, int dim, int indx) const VL_MT_SAFE; }; //=========================================================================== @@ -203,22 +203,22 @@ public: , m_datap{const_cast(datap)} {} ~VerilatedDpiOpenVar() = default; // METHODS - void* datap() const { return m_datap; } + void* datap() const VL_MT_SAFE { return m_datap; } // METHODS - from VerilatedVarProps bool magicOk() const { return m_propsp->magicOk(); } VerilatedVarType vltype() const { return m_propsp->vltype(); } bool isDpiStdLayout() const { return m_propsp->isDpiCLayout(); } const VerilatedRange& packed() const { return m_propsp->packed(); } const VerilatedRange& unpacked() const { return m_propsp->unpacked(); } - int udims() const { return m_propsp->udims(); } - int left(int dim) const { return m_propsp->left(dim); } - int right(int dim) const { return m_propsp->right(dim); } + int udims() const VL_MT_SAFE { return m_propsp->udims(); } + int left(int dim) const VL_MT_SAFE { return m_propsp->left(dim); } + int right(int dim) const VL_MT_SAFE { return m_propsp->right(dim); } int low(int dim) const { return m_propsp->low(dim); } int high(int dim) const { return m_propsp->high(dim); } int increment(int dim) const { return m_propsp->increment(dim); } int elements(int dim) const { return m_propsp->elements(dim); } size_t totalSize() const { return m_propsp->totalSize(); } - void* datapAdjustIndex(void* datap, int dim, int indx) const { + void* datapAdjustIndex(void* datap, int dim, int indx) const VL_MT_SAFE { return m_propsp->datapAdjustIndex(datap, dim, indx); } }; diff --git a/nodist/clang_check_attributes b/nodist/clang_check_attributes index 825e50520..5c6aec2df 100755 --- a/nodist/clang_check_attributes +++ b/nodist/clang_check_attributes @@ -239,6 +239,7 @@ class CallAnnotationsValidator: self._call_location: Optional[FunctionInfo] = None self._caller: Optional[FunctionInfo] = None self._level: int = 0 + self._constructor_context: int = 0 def compile_and_analyze_file(self, source_file: str, compiler_args: list[str], @@ -300,6 +301,67 @@ class CallAnnotationsValidator: annotations = VlAnnotations.from_nodes_list(children) return (True, refd, annotations, children) + def check_mt_safe_call(self, node: clang.cindex.Cursor, + refd: clang.cindex.Cursor, + annotations: VlAnnotations): + is_mt_safe = False + + if annotations.is_mt_safe_call(): + is_mt_safe = True + elif not annotations.is_mt_unsafe_call(): + # Check whether the object the method is called on is mt-safe + def find_object_ref(node): + try: + node = next(node.get_children()) + if node.kind == CursorKind.DECL_REF_EXPR: + # Operator on an argument or local object + return node + if node.kind != CursorKind.MEMBER_REF_EXPR: + return None + if node.referenced and node.referenced.kind == CursorKind.FIELD_DECL: + # Operator on a member object + return node + node = next(node.get_children()) + if node.kind == CursorKind.UNEXPOSED_EXPR: + node = next(node.get_children()) + return node + except StopIteration: + return None + + refn = find_object_ref(node) + if self._constructor_context and not refn: + # we are in constructor and no object reference means + # we are calling local method. It is MT safe + # only if this method is also only calling local methods or + # MT-safe methods + self.iterate_children(refd.get_children(), + self.dispatch_node_inside_definition) + is_mt_safe = True + # class/struct member + elif refn and refn.kind == CursorKind.MEMBER_REF_EXPR and refn.referenced: + refn = refn.referenced + refna = VlAnnotations.from_nodes_list(refn.get_children()) + if refna.guarded or self._constructor_context: + is_mt_safe = True + # variable + elif refn and refn.kind == CursorKind.DECL_REF_EXPR and refn.referenced: + # This is probably a local or an argument. + # Calling methods on local pointers or references is MT-safe, + # but on argument pointers or references is not. + if "*" not in refn.type.spelling and "&" not in refn.type.spelling: + is_mt_safe = True + # local variable + if refn.referenced.kind == CursorKind.VAR_DECL: + is_mt_safe = True + elif refn and refn.kind == CursorKind.CALL_EXPR: + if self._constructor_context: + # call to local function from constructor context + # safe if this function also calling local methods or + # MT-safe methods + self.dispatch_call_node(refn) + is_mt_safe = True + return is_mt_safe + # Call handling def process_method_call(self, node: clang.cindex.Cursor, @@ -310,43 +372,7 @@ class CallAnnotationsValidator: # MT-safe context if ctx.is_mt_safe_context(): - is_mt_safe = False - - if annotations.is_mt_safe_call(): - is_mt_safe = True - elif not annotations.is_mt_unsafe_call(): - # Check whether the object the method is called on is mt-safe - def find_object_ref(node): - try: - node = next(node.get_children()) - if node.kind == CursorKind.DECL_REF_EXPR: - # Operator on an argument or local object - return node - if node.kind != CursorKind.MEMBER_REF_EXPR: - return None - if node.referenced and node.referenced.kind == CursorKind.FIELD_DECL: - # Operator on a member object - return node - node = next(node.get_children()) - if node.kind == CursorKind.UNEXPOSED_EXPR: - node = next(node.get_children()) - return node - except StopIteration: - return None - - refn = find_object_ref(node) - # class/struct member - if refn and refn.kind == CursorKind.MEMBER_REF_EXPR and refn.referenced: - refn = refn.referenced - refna = VlAnnotations.from_nodes_list(refn.get_children()) - if refna.guarded: - is_mt_safe = True - # variable - elif refn and refn.kind == CursorKind.DECL_REF_EXPR and refn.referenced: - # This is probably a local or an argument. Assume it's safe. - is_mt_safe = True - - if not is_mt_safe: + if not self.check_mt_safe_call(node, refd, annotations): self.emit_diagnostic( FunctionInfo.from_node(refd, refd, annotations), DiagnosticKind.NON_MT_SAFE_CALL_IN_MT_SAFE_CTX) @@ -380,7 +406,13 @@ class CallAnnotationsValidator: assert self._call_location ctx = self._call_location.annotations - # Constructors are always OK in MT-safe context. + # Constructors are OK in MT-safe context + # only if they call local methods or MT-safe functions. + if ctx.is_mt_safe_context() or self._constructor_context: + self._constructor_context += 1 + self.iterate_children(refd.get_children(), + self.dispatch_node_inside_definition) + self._constructor_context -= 1 # pure context if ctx.is_pure_context(): @@ -412,6 +444,8 @@ class CallAnnotationsValidator: or refd.kind == CursorKind.CXX_METHOD and refd.is_static_method()): self.process_function_call(refd, annotations) + self.iterate_children(node.get_children(), + self.dispatch_node_inside_definition) return # Function pointer if refd.kind in [ @@ -419,18 +453,26 @@ class CallAnnotationsValidator: CursorKind.PARM_DECL ]: self.process_function_call(refd, annotations) + self.iterate_children(node.get_children(), + self.dispatch_node_inside_definition) return # Non-static class methods if refd.kind == CursorKind.CXX_METHOD: self.process_method_call(node, refd, annotations) + self.iterate_children(node.get_children(), + self.dispatch_node_inside_definition) return # Conversion method (e.g. `operator int()`) if refd.kind == CursorKind.CONVERSION_FUNCTION: self.process_method_call(node, refd, annotations) + self.iterate_children(node.get_children(), + self.dispatch_node_inside_definition) return # Constructors if refd.kind == CursorKind.CONSTRUCTOR: self.process_constructor_call(refd, annotations) + self.iterate_children(node.get_children(), + self.dispatch_node_inside_definition) return # Ignore other callables @@ -461,9 +503,6 @@ class CallAnnotationsValidator: assert refd is not None - prev_caller = self._caller - prev_call_location = self._call_location - def_annotations = VlAnnotations.from_nodes_list(node_children) if not (def_annotations.is_empty() or def_annotations == annotations): @@ -485,8 +524,7 @@ class CallAnnotationsValidator: self.iterate_children(node_children, self.dispatch_node_inside_definition) - self._call_location = prev_call_location - self._caller = prev_caller + self._call_location = None return None @@ -704,8 +742,7 @@ class TopDownSummaryPrinter(): info: FunctionInfo calees: set[FunctionInfo] mismatch: Optional[FunctionInfo] = None - non_mt_safe_call_in_mt_safe: Optional[FunctionInfo] = None - non_pure_call_in_pure: Optional[FunctionInfo] = None + reason: Optional[DiagnosticKind] = None def __init__(self): self._is_first_group = True @@ -727,15 +764,12 @@ class TopDownSummaryPrinter(): if func is None: func = TopDownSummaryPrinter.FunctionCallees(diag.source, set()) self._funcs[usr] = func + func.reason = diag.kind if diag.kind == DiagnosticKind.ANNOTATIONS_DEF_DECL_MISMATCH: func.mismatch = diag.target else: func.calees.add(diag.target) self._unsafe_in_safe.add(diag.target.usr) - if diag.kind == DiagnosticKind.NON_MT_SAFE_CALL_IN_MT_SAFE_CTX: - func.non_mt_safe_call_in_mt_safe = diag.target - elif diag.kind == DiagnosticKind.NON_PURE_CALL_IN_PURE_CTX: - func.non_pure_call_in_pure = diag.target def print_summary(self, root_dir: str): row_groups: dict[str, list[list[str]]] = {} @@ -748,11 +782,11 @@ class TopDownSummaryPrinter(): row_group = [] name = f"\"{func_info.name}\" " - if func.mismatch: + if func.reason == DiagnosticKind.ANNOTATIONS_DEF_DECL_MISMATCH: name += "declaration does not match definition" - elif func.non_mt_safe_call_in_mt_safe: + elif func.reason == DiagnosticKind.NON_MT_SAFE_CALL_IN_MT_SAFE_CTX: name += "is mtsafe but calls non-mtsafe function(s)" - elif func.non_pure_call_in_pure: + elif func.reason == DiagnosticKind.NON_PURE_CALL_IN_PURE_CTX: name += "is pure but calls non-pure function(s)" else: name += "for unknown reason (please add description)" diff --git a/test_regress/t/t_a5_attributes_src.pl b/test_regress/t/t_a5_attributes_src.pl index 2c8fe4e9b..92a4ca5d6 100755 --- a/test_regress/t/t_a5_attributes_src.pl +++ b/test_regress/t/t_a5_attributes_src.pl @@ -36,7 +36,7 @@ sub check { tee => 1, cmd => ["python3", "$root/nodist/clang_check_attributes --verilator-root=$root --cxxflags='$clang_args' $precompile_args $srcfiles_str"]); - file_grep($Self->{run_log_filename}, "Number of functions reported unsafe: 14"); + file_grep($Self->{run_log_filename}, "Number of functions reported unsafe: 27"); } run_clang_check(); diff --git a/test_regress/t/t_dist_attributes_bad.h b/test_regress/t/t_dist_attributes_bad.h index bd24cf77f..5b9ed95b7 100644 --- a/test_regress/t/t_dist_attributes_bad.h +++ b/test_regress/t/t_dist_attributes_bad.h @@ -273,4 +273,114 @@ public: } }; +static void static_function() {} + +class StaticClass { +public: + static void static_class_function() {} +}; + +class ConstructorCallsUnsafeLocalFunction { +public: + void unsafe_function() VL_MT_UNSAFE{}; + ConstructorCallsUnsafeLocalFunction() { unsafe_function(); } +}; +class ConstructorCallsStaticFunctionNoAnnotation { +public: + ConstructorCallsStaticFunctionNoAnnotation() { static_function(); } +}; + +class ConstructorCallsLocalFunction { +public: + void local_function() {} + ConstructorCallsLocalFunction() { local_function(); } +}; + +class ConstructorCallsLocalFunctionCallsGlobal { +public: + void local_function() { static_function(); } + ConstructorCallsLocalFunctionCallsGlobal() { local_function(); } +}; + +class SafeFunction { +public: + void safe_function() VL_MT_SAFE {} +}; +class UnsafeFunction { +public: + void unsafe_function() VL_MT_UNSAFE {} +}; + +class ConstructorWithPointer { +public: + ConstructorWithPointer(SafeFunction* p) { p->safe_function(); } +}; + +class ConstructorWithReference { +public: + ConstructorWithReference(SafeFunction& p) { p.safe_function(); } +}; +class ConstructorWithUnsafePointer { +public: + ConstructorWithUnsafePointer(UnsafeFunction* p) { p->unsafe_function(); } +}; + +class ConstructorWithUnsafeReference { +public: + ConstructorWithUnsafeReference(UnsafeFunction& p) { p.unsafe_function(); } +}; + +class ConstructorCallsLocalCallsGlobal { + void local_function2() { static_function(); } + void local_function() { local_function2(); } + +public: + ConstructorCallsLocalCallsGlobal() { local_function(); } +}; + +class ConstructorCallsLocalCallsClassGlobal { + void local_function2() { StaticClass::static_class_function(); } + void local_function() { local_function2(); } + +public: + ConstructorCallsLocalCallsClassGlobal() { local_function(); } +}; + +class TestClassConstructor { + void safe_function_unsafe_constructor_bad() VL_MT_SAFE { + ConstructorCallsUnsafeLocalFunction f{}; + }; + void safe_function_static_constructor_bad() VL_MT_SAFE { + ConstructorCallsStaticFunctionNoAnnotation f{}; + }; + void safe_function_local_function_global_bad() VL_MT_SAFE { + ConstructorCallsLocalFunctionCallsGlobal f{}; + } + void safe_function_local_function_constructor_good() VL_MT_SAFE { + ConstructorCallsLocalFunction f{}; + } + void safe_function_calls_constructor_with_pointer_good() VL_MT_SAFE { + SafeFunction* i = new SafeFunction{}; + ConstructorWithPointer f{i}; + } + void safe_function_calls_constructor_with_reference_good() VL_MT_SAFE { + SafeFunction i; + ConstructorWithReference f{i}; + } + void safe_function_calls_constructor_with_unsafepointer_bad() VL_MT_SAFE { + UnsafeFunction* i = new UnsafeFunction{}; + ConstructorWithUnsafePointer f{i}; + } + void safe_function_calls_constructor_with_unsafereference_bad() VL_MT_SAFE { + UnsafeFunction i; + ConstructorWithUnsafeReference f{i}; + } + void safe_function_calls_constructor_local_calls_global_bad() VL_MT_SAFE { + ConstructorCallsLocalCallsGlobal f{}; + } + void safe_function_calls_constructor_local_calls_class_global_bad() VL_MT_SAFE { + ConstructorCallsLocalCallsClassGlobal f{}; + } +}; + #endif // T_DIST_ATTRIBUTES_BAD_H_ diff --git a/test_regress/t/t_dist_attributes_bad.out b/test_regress/t/t_dist_attributes_bad.out index 71a4bd01b..e7649a02f 100644 --- a/test_regress/t/t_dist_attributes_bad.out +++ b/test_regress/t/t_dist_attributes_bad.out @@ -724,6 +724,34 @@ t/t_dist_attributes_bad.cpp:75: [release] TestClass t/t_dist_attributes_bad.h:124: [] TestClass::scm_ua_VL_REQUIRES(VerilatedMutex &) [declaration] t/t_dist_attributes_bad.cpp:75: [requires] TestClass::scm_ua_VL_REQUIRES(VerilatedMutex &) +%Error: "TestClassConstructor::safe_function_calls_constructor_local_calls_class_global_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:381: [mt_safe] TestClassConstructor::safe_function_calls_constructor_local_calls_class_global_bad() +t/t_dist_attributes_bad.h:280: [] StaticClass::static_class_function() + +%Error: "TestClassConstructor::safe_function_calls_constructor_local_calls_global_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:378: [mt_safe] TestClassConstructor::safe_function_calls_constructor_local_calls_global_bad() +t/t_dist_attributes_bad.h:276: [] static_function() + +%Error: "TestClassConstructor::safe_function_calls_constructor_with_unsafepointer_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:370: [mt_safe] TestClassConstructor::safe_function_calls_constructor_with_unsafepointer_bad() +t/t_dist_attributes_bad.h:311: [mt_unsafe] UnsafeFunction::unsafe_function() + +%Error: "TestClassConstructor::safe_function_calls_constructor_with_unsafereference_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:374: [mt_safe] TestClassConstructor::safe_function_calls_constructor_with_unsafereference_bad() +t/t_dist_attributes_bad.h:311: [mt_unsafe] UnsafeFunction::unsafe_function() + +%Error: "TestClassConstructor::safe_function_local_function_global_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:356: [mt_safe] TestClassConstructor::safe_function_local_function_global_bad() +t/t_dist_attributes_bad.h:276: [] static_function() + +%Error: "TestClassConstructor::safe_function_static_constructor_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:353: [mt_safe] TestClassConstructor::safe_function_static_constructor_bad() +t/t_dist_attributes_bad.h:276: [] static_function() + +%Error: "TestClassConstructor::safe_function_unsafe_constructor_bad()" is mtsafe but calls non-mtsafe function(s) +t/t_dist_attributes_bad.h:350: [mt_safe] TestClassConstructor::safe_function_unsafe_constructor_bad() +t/t_dist_attributes_bad.h:285: [mt_unsafe] ConstructorCallsUnsafeLocalFunction::unsafe_function() + %Error: "ifh_test_caller_func_VL_MT_SAFE(VerilatedMutex &)" is mtsafe but calls non-mtsafe function(s) t/t_dist_attributes_bad.cpp:53: [mt_safe] ifh_test_caller_func_VL_MT_SAFE(VerilatedMutex &) t/t_dist_attributes_bad.h:94: [] ifh_NO_ANNOTATION(VerilatedMutex &) @@ -1157,4 +1185,4 @@ t/t_dist_attributes_bad.cpp:60: [mt_unsafe_one] sfc_VL_ t/t_dist_attributes_bad.cpp:60: [release] sfc_VL_RELEASE(VerilatedMutex &) t/t_dist_attributes_bad.cpp:60: [release] sfc_VL_RELEASE_SHARED(VerilatedMutex &) t/t_dist_attributes_bad.cpp:60: [requires] sfc_VL_REQUIRES(VerilatedMutex &) -Number of functions reported unsafe: 220 +Number of functions reported unsafe: 224