ariccio added a comment.
Nevermind, the order is correct!
https://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
In https://reviews.llvm.org/D18073#504216, @dcoughlin wrote:
> No. The identifier info will be null for C++ operators.
I assume you mean `operator new/new[]/delete/delete[]
> > Thus, when`! isWindowsMSVCEnvironment`, I leave the Windows-only memory
> > allocating func
ariccio added a comment.
Bump?
https://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
Bump?
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
I should elaborate. The principle of operation of this latest patch is that the
`FunctionDecl` in `IsCMemFunction` should never return a `nullptr`
`IdentifierInfo*` from `getIdentifier` (is that a valid assumption?)... Thus,
when`! isWindowsMSVCEnvironment`, I leave the
ariccio updated this revision to Diff 55774.
ariccio added a comment.
Only check for MSVC-specific functions when actually building FOR MSVC (i.e.
`isWindowsMSVCEnvironment`).
Sidenote: is there any reason why none of the `ASTContext&`s are `const`ified
in this file?
http://reviews.llvm.org/D
ariccio marked an inline comment as done.
ariccio added a comment.
Wrongly `#if defined` out test is now fixed.
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe
ariccio updated this revision to Diff 55366.
ariccio added a comment.
Conditionally check Windows methods on Windows.
Is this what you're thinking of? It's kinda ugly - I was hoping to avoid the
`#if`s - but it does only check the Windows functions on Windows builds only.
http://reviews.llvm.o
ariccio updated this revision to Diff 53995.
ariccio added a comment.
So, duh, it turns out I //can// use `_WIN32` to conditionally test.
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/alternative-malloc-ap
ariccio added a comment.
In http://reviews.llvm.org/D18073#398882, @zaks.anna wrote:
> "Since we are adding support for so many new APIs that are only available on
> Windows, could you please condition checking them only when we build for
> Windows. You probably can look and Language Options to
ariccio updated this revision to Diff 53495.
ariccio added a comment.
Changed the new file name.
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/alternative-malloc-api.c
llvm/tools/clang/test/Analysis/mall
ariccio updated this revision to Diff 53193.
ariccio added a comment.
I'm not actually sure why I didn't want to split these off into a separate
file, but now I finally have. Is this what you were looking for?
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers
ariccio added a comment.
In http://reviews.llvm.org/D18073#393613, @zaks.anna wrote:
> You will have to add one test function to smoke test that the newly added API
> is modeled correctly.
Isn't that what I've already done?
> We also have a lot of existing tests that verify that each of the o
ariccio updated this revision to Diff 52876.
ariccio added a comment.
Fewer added test functions.
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/malloc.c
Index: llvm/tools/clang/test/Analysis/malloc.c
ariccio added a comment.
In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote:
> > So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?
>
>
> Yes.
Ok, that I can do. Will upload patch later this afternoon/tonight.
In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote:
> Also,
ariccio added a comment.
Maybe I'm being thick headed and I can't see it - sorry if I am - but I'm still
a bit confused. Can you tell me what I should do in the `_wcsdup_dbg` example?
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-
ariccio added a comment.
So for `_wcsdup_dbg`, I should leave only `testWinWcsdupDbg`?
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
Here's my confusion: don't we //kinda// have to duplicate tests? After all
these functions are //essentially// duplicate functions.
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http
ariccio added a comment.
I'm sorry, I'm confused.
As an example, for `_wcsdup_dbg`, I've added `testWinWcsdupDbg`,
`testWinWcsdupDbgContentIsDefined`, and `testWinWideDbgLeakWithinReturn`. Which
ones should I drop for that API?
http://reviews.llvm.org/D18073
___
ariccio added a comment.
In http://reviews.llvm.org/D18498#384711, @mspertus wrote:
> I installed both VS2015 and VS2013 on my laptop and saw the correct solution
> files built by cmake.
Btw, make sure you're running the latest updates for both Visual Studio, else
you'll get funny build error
ariccio added inline comments.
Comment at: utils/ClangVisualizers/CMakeLists.txt:3
@@ +2,3 @@
+# tries to create a corresponding executable, which we don't want.
+if (LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION)
+ set(CLANG_VISUALIZERS clang.natvis)
Hmm, that's gonna
ariccio added a comment.
In http://reviews.llvm.org/D18497#384362, @mspertus wrote:
> In http://reviews.llvm.org/D18497#384351, @ariccio wrote:
>
> > Otherwise, assuming everything builds correctly, LGTM.
>
>
> There are a lot scenarios. What do all of these files in the build tree like
> `cmake
ariccio added a comment.
Assuming everything builds correctly, LGTM.
Your CMake is better than mine, so I'm not sure if there're better ways to do
this ;)
Comment at: utils/ClangVisualizers/CMakeLists.txt:2
@@ +1,3 @@
+# Do this by hand instead of using add_llvm_utilities(), w
ariccio added a comment.
In http://reviews.llvm.org/D18498#384274, @zturner wrote:
> Ahh that's surprising. If it works even with the none tag, i guess my
> concerns are not valid
I'm only partly surprised... It's not the first time that a Microsoft product
behaves surprisingly :)
http://re
ariccio added a comment.
`utils/llvm.natvis` is a duplicate of `utils/LLVMVisualizers/llvm.natvis`?
Otherwise, assuming everything builds correctly, LGTM.
Once again, thanks for doing this!
http://reviews.llvm.org/D18497
___
cfe-commits mailing li
ariccio marked an inline comment as done.
ariccio added a comment.
In http://reviews.llvm.org/D16873#371736, @a.sidorin wrote:
> Alexander, could you update status of this review?
It's become a zombie review. I should just abandon it, as I'll be away for two
weeks starting tomorrow.
http://r
ariccio added a comment.
In http://reviews.llvm.org/D18073#372697, @zaks.anna wrote:
> Since we are adding support for so many new APIs that are only available on
> Windows, could you please condition checking them only when we build for
> Windows. You probably can look and Language Options to
ariccio created this revision.
ariccio added reviewers: zaks.anna, dcoughlin, aaron.ballman.
ariccio added a subscriber: cfe-commits.
As discussed...
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/malloc.c
ariccio updated this revision to Diff 50241.
ariccio added a comment.
For some reason, this smaller diff was harder to upload than the first diff.
http://reviews.llvm.org/D17983
Files:
llvm/lib/Analysis/DependenceAnalysis.cpp
llvm/lib/Analysis/InstructionSimplify.cpp
llvm/lib/AsmParser/LL
ariccio added a comment.
Oh, and by the way, what's the policy on using `enum class`es instead of C
style enums? I bet the compiler would have fewer false positives with strongly
typed enums?
http://reviews.llvm.org/D17983
___
cfe-commits mailing
ariccio added a comment.
In http://reviews.llvm.org/D17983#370717, @dblaikie wrote:
> Initializations we never expect to use (eg because we have a covered switch
> that initializes in all cases, or some slightly complex control flow the
> compiler can't see through) hinder our ability to find u
ariccio created this revision.
ariccio added subscribers: llvm-commits, cfe-commits.
Herald added a reviewer: tstellarAMD.
Herald added subscribers: joker.eph, dsanders, arsenm, MatzeB.
Currently, the "potentially uninitialized local variable" & "potentially
uninitialized local pointer variable"
ariccio added a subscriber: ariccio.
ariccio added a comment.
Side note: Has anybody ever considered just treating `fopen` and `fclose` like
`malloc` and `free`?
On Windows I use a trick that I discovered what I call "the
`_Post_ptr_invalid_` hack" because* the `_Post_ptr_invalid_` SAL annotati
ariccio added a comment.
In http://reviews.llvm.org/D17688#366780, @zaks.anna wrote:
> LGTM. Thanks!
>
> I can commit this in your behalf.
Oh, and yeah, I don't have privs.
http://reviews.llvm.org/D17688
___
cfe-commits mailing list
cfe-commits@l
ariccio added a comment.
In http://reviews.llvm.org/D17688#368330, @zaks.anna wrote:
>
?
http://reviews.llvm.org/D17688
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio updated this revision to Diff 49845.
ariccio added a comment.
Alrighty. This final version of the patch causes no test failures (vs the
unmodified build*).
*An unrelated test normally fails on my machine.
http://reviews.llvm.org/D17688
Files:
llvm/tools/clang/lib/StaticAnalyzer/Chec
ariccio added a comment.
Hmm. This seems to be because `wchar_t` is enabled by `-fms-compatibility`.
@aaron.ballman do you have any idea how to define `wchar_t` without
`-fms-compatibility`?
http://reviews.llvm.org/D17688
___
cfe-commits mailing l
ariccio added a comment.
In http://reviews.llvm.org/D17688#366883, @zaks.anna wrote:
> Please, do not submit patches for review before they pass all tests.
Whoops, sorry. Should I been a bit clearer that check-all hadn't finished when
I updated the diff, or should I not update the diff at all
ariccio added a comment.
Hmm, check-all produced a number of issues, which I think stem from the
following warning:
File C:\LLVM\llvm\tools\clang\test\Analysis\malloc.c Line 16: unknown type
name 'wchar_t'
(with a bunch of instances of that)
...so how do I declare/define `wchar_t` properly?
ariccio updated this revision to Diff 49674.
ariccio added a comment.
Added newly checked variants to the malloc checks.
(Running the check-all suite now, will update with results)
http://reviews.llvm.org/D17688
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/too
ariccio added a comment.
In http://reviews.llvm.org/D17688#366011, @ariccio wrote:
> In http://reviews.llvm.org/D17688#365951, @zaks.anna wrote:
>
> > ls ./clang/test/Analysis/malloc*
>
>
> Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the
> beginning:
>
> #if defined(_WI
ariccio added a comment.
In http://reviews.llvm.org/D17688#365951, @zaks.anna wrote:
> ls ./clang/test/Analysis/malloc*
Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the
beginning:
#if defined(_WIN32)
#define strdup _strdup
#define alloca _alloca
#endif //d
ariccio added a comment.
In http://reviews.llvm.org/D17688#365880, @zaks.anna wrote:
> I suggest to update the malloc regression test with these.
Eh? There's a malloc regression test?
http://reviews.llvm.org/D17688
___
cfe-commits mailing list
cf
> I just don't understand why some functions made it in this patch and not
> others (notably, why the lack of _mbsdup, which is documented on the same
> page as others you are adding).
It's partly because of the better wrapper class that was mentioned (in a
discussion that I'm having trouble fi
ariccio added a comment.
Is this patch all clear to go?
I hope I don't sound too pushy - I just don't want to lose momentum here.
http://reviews.llvm.org/D17688
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
ariccio updated this revision to Diff 49456.
ariccio added a comment.
Nit addressed.
http://reviews.llvm.org/D17688
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
ariccio added a comment.
//(This is mostly for my own reference)//
BTW, there are a few other non-MS-only functions in the C standard library that
allocate memory that needs to be free()d:
1. getcwd (and _getcwd/_wgetcwd)
And some MS-specific functions that I'm surprised are actually MS-only:
ariccio updated this revision to Diff 49330.
ariccio added a comment.
Changed underscore prefixed variable names to `win` prefixed variable names.
http://reviews.llvm.org/D17688
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Index: llvm/tools/clang/lib/StaticAnalyzer/C
ariccio added a comment.
In http://reviews.llvm.org/D17688#363835, @zaks.anna wrote:
> The two underscores in the names are hard to read. Maybe we should just
> prefix them with 'win'?
I like that better, will change.
http://reviews.llvm.org/D17688
ariccio created this revision.
ariccio added a subscriber: cfe-commits.
I've found & fixed a leak that Clang misses when compiling on Windows.
The leak was found by [[
https://samate.nist.gov/SARD/view_testcase.php?tID=149071 | SARD #149071 ]],
mem1-bad.c. Clang misses it because MSVC uses _str
ariccio added inline comments.
Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:521
@@ -502,3 +520,3 @@
SummaryLog.write("See the first %d below.\n"
- % (NumOfFailuresInSummary,))
+
ariccio added a comment.
I've added a few comments where I think the changes are not quite clear.
Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/CmpRuns.py:159
@@ -147,3 +158,3 @@
if 'clang_version' in data:
-if self.clang_version == None:
+
ariccio added inline comments.
Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/CmpRuns.py:194
@@ -182,3 +193,3 @@
# Backward compatibility API.
-def loadResults(path, opts, root = "", deleteEmpty=True):
+def loadResults(path, opts, root="", deleteEmpty=True):
return load
ariccio added a comment.
In http://reviews.llvm.org/D17253#353000, @zaks.anna wrote:
> Not sure how you got these changes, but some of them seem wrong and some seem
> inconsistently applied.
>
> Has this been tested?
Not yet, I was toying with running the first SARD test under it, and read
th
ariccio added a comment.
Whoops, forgot to submit my earlier comments. Responding to comments right
now...
Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/CmpRuns.py:31
@@ -30,3 +30,3 @@
import plistlib
-import CmpRuns
+import CmpRuns # ?
This file impo
ariccio added a comment.
In http://reviews.llvm.org/D17130#349744, @craig.topper wrote:
> What's complex about the SVal constructors?
I arbitrarily figured that classes that are more than twice-derived (is there a
better way to say that) are complex. I don't think there was any
//particularly
ariccio added inline comments.
Comment at:
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:143
@@ -144,3 +142,3 @@
void addAbortedBlock(const ExplodedNode *node, const CFGBlock *block) {
-blocksAborted.push_back(std::make_pair(block, node));
+
ariccio updated the summary for this revision.
ariccio updated this revision to Diff 47603.
http://reviews.llvm.org/D17130
Files:
llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
llvm/tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
llvm/tools/clang
ariccio created this revision.
ariccio added a subscriber: cfe-commits.
As discussed in "Code in headers" on llvm-dev, there are lots of headers with
complex code in them. I've moved some complex constructors & destructors to
implementation files, using [[
https://www.chromium.org/developers/co
ariccio marked 2 inline comments as done.
ariccio added a comment.
I've reformatted, and changed the `PointerUnion` to a `const
StackFrameContext*`.
Built successfully, etc...
(I hope I don't sound too pushy!)
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
ariccio updated this revision to Diff 47289.
http://reviews.llvm.org/D16873
Files:
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
Index: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRe
ariccio added a comment.
One of two comments addressed, one question asked.
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ +791,3 @@
+
+const MemRegion* MemRegionManager::getMemSpaceForLocalVariable(const VarDecl
*D, llvm::PointerUnion &V) {
+ const
ariccio added a comment.
All clear. Ready for landing?
http://reviews.llvm.org/D16873
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio updated this revision to Diff 47161.
ariccio added a comment.
Responded to comments (fixed the bug noticed in review), and changed names.
http://reviews.llvm.org/D16873
Files:
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
llvm/tools/clang/lib/StaticAna
ariccio marked 3 inline comments as done.
ariccio added a comment.
Alrighty, will update the diff momentarily.
http://reviews.llvm.org/D16873
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
ariccio added a comment.
Bump?
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ -783,12 +791,3 @@
-// Treat other globals as GlobalInternal unless they are constants.
-} else {
- QualType GQT = D->getType();
- const Type *GT = GQT.get
ariccio marked 11 inline comments as done.
ariccio added a comment.
Marked some inline comments as done.
http://reviews.llvm.org/D16873
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit
ariccio marked 9 inline comments as done.
ariccio added a comment.
Whoops, I didn't //submit// my comments.
Comment at:
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1185
@@ +1184,3 @@
+ /// getMemRegionGloballyStored - Retrieve or create the mem
ariccio added a comment.
Responded to comments.
Will happily make changes when questions are answered.
Comment at:
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1186
@@ +1185,3 @@
+ /// associated with a specified globally stored, non-static lo
ariccio created this revision.
ariccio added reviewers: aaron.ballman, dcoughlin.
ariccio added a subscriber: cfe-commits.
Instead of one long function, `MemRegionManager::getVarRegion` now calls
`getMemRegionGloballyStored` for global, non-static variables, and
`getMemRegionStaticLocal` for sta
ariccio added a comment.
In http://reviews.llvm.org/D13893#343436, @angelgarcia wrote:
> The compiler complained about creating constant instances of classes
> without a user provided constructor (which is the case for the ASTMatchers).
>
> I gave up this change because it broke the build for a
ariccio marked an inline comment as done.
ariccio added a comment.
Grr! missed a single note.
http://reviews.llvm.org/D16748
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
See original diff here: http://reviews.llvm.org/D13890
Repository:
rL LLVM
http://reviews.llvm.org/D13893
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a subscriber: ariccio.
ariccio added a comment.
Whatever happened to this? What in `ASTMatchers` did it break? It seems like a
good change, and nobody followed up to fix it?
Repository:
rL LLVM
http://reviews.llvm.org/D13893
___
cf
ariccio added a comment.
In http://reviews.llvm.org/D16748#343011, @aaron.ballman wrote:
> LGTM, but the patch does not apply cleanly because it was created using
> absolute paths. In the future, please generate the patch with relative paths.
> ;-)
>
> Commit in r259652
Oops.
http://reviews
ariccio added a comment.
All set.
http://reviews.llvm.org/D16748
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio updated this revision to Diff 46737.
ariccio marked 2 inline comments as done.
ariccio added a comment.
Removed single `const`.
http://reviews.llvm.org/D16748
Files:
C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
C:/LLVM/llvm/tools/clang/lib/St
ariccio marked 3 inline comments as done.
ariccio added a comment.
I will remove that `const` later tonight.
Comment at: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1396
@@ -1395,3 +1395,3 @@
const auto &ReferencedBlockVars = AC->getReferencedBlockVars(BC->
ariccio marked an inline comment as done.
ariccio added a comment.
As said in comment, I disagree with the no need for `const` here.
Comment at: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1396
@@ -1395,3 +1395,3 @@
const auto &ReferencedBlockVars = AC->get
ariccio updated this revision to Diff 46481.
ariccio marked 10 inline comments as done.
ariccio added a comment.
Removed extra spaces.
http://reviews.llvm.org/D16748
Files:
C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
C:/LLVM/llvm/tools/clang/lib/Sta
ariccio added a comment.
Oops. The funny thing about the extra spaces is that I wasn't sure why there
were spaces after the casts already, so I figured that I should keep it that
way.
I shall fix.
http://reviews.llvm.org/D16748
___
cfe-commits m
81 matches
Mail list logo
