Page MenuHomePhabricator

Add memory allocating functions
Needs ReviewPublic

Authored by ariccio on Mar 10 2016, 5:31 PM.

Details

Summary

As discussed...

Diff Detail

Event Timeline

ariccio updated this revision to Diff 50384.Mar 10 2016, 5:31 PM
ariccio retitled this revision from to Add memory allocating functions.
ariccio updated this object.
ariccio added a subscriber: cfe-commits.
zaks.anna edited edge metadata.Mar 10 2016, 5:44 PM

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 figure that out.

Also, we should not duplicate all of our tests. Instead, we should add a single test per added API checking that it is modeling the operation we think it should model.

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 figure that out.

Is there a problem with checking them on linux/OSX also?

Also, we should not duplicate all of our tests. Instead, we should add a single test per added API checking that it is modeling the operation we think it should model.

So should I dump the _dbg versions from the tests?

Also, we should not duplicate all of our tests. Instead, we should add a single test per added API checking that it is modeling the operation we think it
should model.

So should I dump the _dbg versions from the tests?

No but do not duplicate all of the tests. Please, add a single test per API you are adding.

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?

Here's my confusion: don't we kinda have to duplicate tests? After all these functions are essentially duplicate functions.

Can you add one test per new function, which tests that the function if modeled correctly. Please, do not duplicate all tests that contain a function with it's windows variants.

Hope this is more clear.

So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?

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?

So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?

Yes.

Also, since there will be many of these "alternate" functions, could you create a separate test file for them?

So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?

Yes.

Ok, that I can do. Will upload patch later this afternoon/tonight.

Also, since there will be many of these "alternate" functions, could you create a separate test file for them?

But, that contradicts removing them? Huh?

You will have to add one test function to smoke test that the newly added API is modeled correctly. We also have a lot of existing tests that verify that each of the original APIs (malloc. free, new) function correctly in all possible settings. What is the contradiction in asking to add the smoke tests in a separate file?

ariccio updated this revision to Diff 52876.Apr 6 2016, 5:51 PM
ariccio edited edge metadata.

Fewer added test functions.

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 original APIs (malloc. free, new) function correctly in all possible settings.
What is the contradiction in asking to add the smoke tests in a separate file?

Ahh, I was confused as to whether you wanted me to remove them entirely - moving them to a different file is self-consistent. I'm not exactly sure of the benefit of moving these tests to a separate file?

Sidenote: As someone who has worked a little bit with electronics and simple hardware design & construction, I love this use of "smoke test".

ariccio updated this revision to Diff 53193.Apr 10 2016, 11:18 PM

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?

"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 figure that out."

malloc-uses.c -> "alternative-malloc-api.c" ?

ariccio updated this revision to Diff 53495.Apr 12 2016, 4:58 PM

Changed the new file name.

"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 figure that out."

malloc-uses.c -> "alternative-malloc-api.c" ?

Sorry for being so thickheaded about that the first go-around, I'm not entirely sure what was up with me. I've changed the name of the file, but I haven't yet figured out how to set up the Windows only tests to run on Windows only. I'm assuming I can't simply:

#if defined(_WIN32)

[...]

#endif

...because lit doesn't define _WIN32 for me. Which Language Options should I look in?

aaron.ballman edited edge metadata.Apr 14 2016, 9:20 AM

"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 figure that out."

malloc-uses.c -> "alternative-malloc-api.c" ?

Sorry for being so thickheaded about that the first go-around, I'm not entirely sure what was up with me. I've changed the name of the file, but I haven't yet figured out how to set up the Windows only tests to run on Windows only. I'm assuming I can't simply:

#if defined(_WIN32)

[...]

#endif

...because lit doesn't define _WIN32 for me. Which Language Options should I look in?

I believe that you can specify a triple using -triple i386-pc-win32 or something similar to force the compilation to be for a Windows target.

ariccio updated this revision to Diff 53995.Apr 16 2016, 3:29 PM
ariccio edited edge metadata.

So, duh, it turns out I can use _WIN32 to conditionally test.

This looks reasonable to me, but you should wait for @zaks.anna to sign off.

"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 figure that out."

By this, I was suggesting that we should be conditionally checking for Windows functions in the checker, not only the tests. Are these all of the Windows-specific functions that will be added to the Malloc checker or do you plan on adding more? If there are more variants, I definitely think we should conditionally check (in the checker).

Regarding tests, they should reflect what is in the checker. Currently, the checker will support '_mbsdup' on all architectures, but the tests only check it on Windows.

llvm/tools/clang/test/Analysis/malloc.c
1576

This is not Windows-only!

ariccio updated this revision to Diff 55366.Apr 27 2016, 6:05 PM

Conditionally check Windows methods on Windows.

Is this what you're thinking of? It's kinda ugly - I was hoping to avoid the #ifs - but it does only check the Windows functions on Windows builds only.

ariccio marked an inline comment as done.Apr 27 2016, 6:06 PM

Wrongly #if defined out test is now fixed.

You conditionally defined when you build ON Windows machine, not when you build FOR Windows. You should be able to query the compiler to check which targets it's building for.

ariccio updated this revision to Diff 55774.May 1 2016, 9:48 PM

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 constified in this file?

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 Windows-only memory allocating functions initialized to nullptr, which will never equal a non-null IdentifierInfo*, and never trigger on a non-Windows platform.

dcoughlin edited edge metadata.Aug 2 2016, 4:22 PM

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?)

No. The identifier info will be null for C++ operators.

Thus, when`! isWindowsMSVCEnvironment`, I leave the Windows-only memory allocating functions initialized to nullptr, which will never equal a non-null IdentifierInfo*, and never trigger on a non-Windows platform.

It is probably better to be explicit about this.

llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
0–1

Can you initialized these in the order the fields are declared? This will avoid warnings with '-Wreorder' enabled.

llvm/tools/clang/test/Analysis/alternative-malloc-api.c
3

You should add a second RUN line here with the "-triple x86_64-windows -fms-compatibility" flags added to test analyzing with a windows target on all host platforms.

dcoughlin requested changes to this revision.Aug 4 2016, 3:21 PM
dcoughlin edited edge metadata.
This revision now requires changes to proceed.Aug 4 2016, 3:21 PM
ariccio added a comment.EditedAug 8 2016, 5:13 PM

No. The identifier info will be null for C++ operators.

I assume you mean operator new/new[]/delete/delete[] by C++ operators? If so, maybe I can FD->getOverloadedOperator() and then check it against OO_New, OO_Array_New, OO_Delete, & OO_Array_Delete?

Edit: isStandardNewDelete would do that work for me.

ariccio updated this revision to Diff 76993.Nov 6 2016, 2:10 PM
ariccio edited edge metadata.
ariccio marked an inline comment as done.

I think this addresses the previous issues, and it passes all tests, although I haven't yet fixed the wreorder issue (I just missed it, and I'mma rerun tests in a second).

Nevermind, the order is correct!

Thanks for iterating on the patch! Some comments in-line.

llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
569

Please add a space here to align with the rest of the comment.

622

The identifier info is null for other operators as well (e.g., +).
If you want to bail early when the identifier for a function is null, you should do so directly instead of trying to enumerate all cases.

For example:

if (!FunI)
  return false;

This will be future-proof in case additional additional kinds of function declarations are added without identifiers.

648

The indentation seems off here.

664

You should remove this assert.

Since you have guaranteed that you are targeting windows before checking and you initially II_win_alloca to a non-null value when targeting windows it can never be the case that II_win_alloca will be null here.

814

Do you want to do the same early bailout for an operator here also?

850

I don't think the !isStandardNewDelete() is needed here.

Also, this is triggering -Wlogical-op-parentheses for me when building with clang.

859

Same here.

866

Same here.

874

Same here.

881

Same here.

1183

You should remove the commented-out code.

1185

The explanation in the assert is not correct. This invariant holds because Sema guarantees that the module is not null when checking the attribute. See handleOwnershipAttr() for details.

1191

The '!isStandardNewDelete` check doesn't make any sense here. The code is dealing with the identifier in specified attribute and not the name of the called function.

1285

Same here.

llvm/tools/clang/test/Analysis/alternative-malloc-api.c
88

I don't know the answer to this. Perhaps @zaks.anna recalls -- it looks like she wrote the test that this was copied from. But I don't think this comment adds much, do you?

95

Same here.