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

[analyzer][NFC] Remove outdated FIXME comment #68211

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

NagyDonat
Copy link
Contributor

This trivial commit removes a 13-year-old FIXME comment from MallocChecker.cpp because it was fixed at least 10 years ago when getConjuredHeapSymbolVal() was added.

This trivial commit removes a 13-year-old FIXME comment from
MallocChecker.cpp because it was fixed at least 10 years ago when
`getConjuredHeapSymbolVal()` was added.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Oct 4, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2023

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Changes

This trivial commit removes a 13-year-old FIXME comment from MallocChecker.cpp because it was fixed at least 10 years ago when getConjuredHeapSymbolVal() was added.


Full diff: https://github.com/llvm/llvm-project/pull/68211.diff

1 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+4-7)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 18c7f3e4f6e6b92..d3a4020280616b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1961,13 +1961,10 @@ ProgramStateRef MallocChecker::FreeMemAux(
   // Parameters, locals, statics, globals, and memory returned by
   // __builtin_alloca() shouldn't be freed.
   if (!isa<UnknownSpaceRegion, HeapSpaceRegion>(MS)) {
-    // FIXME: at the time this code was written, malloc() regions were
-    // represented by conjured symbols, which are all in UnknownSpaceRegion.
-    // This means that there isn't actually anything from HeapSpaceRegion
-    // that should be freed, even though we allow it here.
-    // Of course, free() can work on memory allocated outside the current
-    // function, so UnknownSpaceRegion is always a possibility.
-    // False negatives are better than false positives.
+    // Regions returned by malloc() are represented by SymbolicRegion objects
+    // within HeapSpaceRegion. Of course, free() can work on memory allocated
+    // outside the current function, so UnknownSpaceRegion is also a
+    // possibility here.
 
     if (isa<AllocaRegion>(R))
       HandleFreeAlloca(C, ArgVal, ArgExpr->getSourceRange());

Copy link
Contributor

@gamesh411 gamesh411 left a comment

Choose a reason for hiding this comment

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

I can confirm the claims about HeapSymRegion being used:
image

@NagyDonat NagyDonat merged commit 2856e72 into llvm:main Oct 9, 2023
4 checks passed
@NagyDonat NagyDonat deleted the remove_malloc_fixme branch October 9, 2023 11:56
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

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

LGTM Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants