This is an archive of the discontinued LLVM Phabricator instance.

Fix missed leak from MSVC specific allocation functions
ClosedPublic

Authored by ariccio on Feb 27 2016, 9:56 PM.

Details

Summary

I've found & fixed a leak that Clang misses when compiling on Windows.

The leak was found by SARD #149071, mem1-bad.c. Clang misses it because MSVC uses _strdup instead of strdup. I've added an IdentifierInfo* for _strdup, and now Clang catches it.

As a drive by fix, I added the wide character strdup variants (wcsdup, _wcsdup) and the MSVC version of alloca (_alloca).

No new check-all failures. (Did somebody improve lit's output?) Adding reviewers...

Diff Detail

Repository
rL LLVM

Event Timeline

ariccio updated this revision to Diff 49308.Feb 27 2016, 9:56 PM
ariccio retitled this revision from to Fix missed leak from MSVC specific allocation functions.
ariccio updated this object.
ariccio added a subscriber: cfe-commits.
ariccio updated this object.Feb 27 2016, 11:10 PM
zaks.anna edited edge metadata.Feb 28 2016, 10:21 AM

The two underscores in the names are hard to read. Maybe we should just prefix them with 'win'?

The two underscores in the names are hard to read. Maybe we should just prefix them with 'win'?

I like that better, will change.

(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:

  1. _getdcwd/_wgetdcwd
  2. _dupenv_s/_wdupenv_s
  3. _fullpath/_wfullpath
ariccio updated this revision to Diff 49330.Feb 28 2016, 3:08 PM
ariccio edited edge metadata.

Changed underscore prefixed variable names to win prefixed variable names.

Nit: Could you change the prefix from "win" to "win_"?

ariccio updated this revision to Diff 49456.Feb 29 2016, 11:16 PM

Nit addressed.

Is this patch all clear to go?

I hope I don't sound too pushy - I just don't want to lose momentum here.

I suggest to update the malloc regression test with these.

I suggest to update the malloc regression test with these.

Eh? There's a malloc regression test?

Of cause, we have regression tests for (almost) everything:
ls ./clang/test/Analysis/malloc*

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 //defined(_WIN32)

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 //defined(_WIN32)

Nevermind. I'm uploading a proper patch in the next few minutes.

ariccio updated this revision to Diff 49674.Mar 2 2016, 2:58 PM

Added newly checked variants to the malloc checks.

(Running the check-all suite now, will update with results)

zaks.anna accepted this revision.Mar 2 2016, 3:07 PM
zaks.anna edited edge metadata.

LGTM. Thanks!

I can commit this in your behalf.

This revision is now accepted and ready to land.Mar 2 2016, 3:07 PM

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?

Generate a preprocessed file and keep copying the definitions from there until you reach the types compiler knows about.

Please, do not submit patches for review before they pass all tests.

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 until check-all finishes?

The second: you should not update the diff. When a patch is uploaded, the assumption is that all tests pass:)

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?

ariccio updated this revision to Diff 49845.Mar 4 2016, 1:09 PM
ariccio edited edge metadata.

Alrighty. This final version of the patch causes no test failures (vs the unmodified build*).

*An unrelated test normally fails on my machine.

LGTM. Thanks!

I can commit this in your behalf.

Oh, and yeah, I don't have privs.

This revision was automatically updated to reflect the committed changes.