[PATCH] D129280: [analyzer] PlacementNewChecker, properly handle array overhead (cookie)

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:157
             "Storage provided to placement new is only {0} bytes, "
-            "whereas the allocated array type requires more space for "
-            "internal needs",
-            SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+            "whereas the allocated array type might require more space for "
+            "allocation overhead",
----------------
NoQ wrote:
> isuckatcs wrote:
> > martong wrote:
> > > NoQ wrote:
> > > > "might" is not very convincing, it may cause a reaction like "I've no idea what it's talking about and the compiler itself isn't sure, must be false positive". Can we do anything to point the user in the right direction? Say, if this is implementation-defined, are we looking at a portability issue?
> > > Okay, I see your point. Let's dissect the corresponding sections of the standard:
> > > ```
> > > new(2, f) T[5] results in a call of operator new[](sizeof(T) * 5 + y, 2, f).
> > > 
> > > Here, ... and y are non-negative unspecified values representing array allocation overhead;
> > > ```
> > > The array overhead is an **unspecified value**. What does it mean exactly? My understanding is that this means it is implementation defined and the implementation is not required to document or guarantee anything. I came to this conclusion based on this [[ https://stackoverflow.com/questions/2397984/undefined-unspecified-and-implementation-defined-behavior | stackoverflow question ]]. 
> > > 
> > > My interpretation could be wrong, but that does not matter. I think, we should just simply display the user what the standard says, and let them digest themselves the meaning of "unspecified". I am updating the patch like so.
> > I also looked into this overhead question. In this [[ https://stackoverflow.com/a/8721932 | stackoverflow answer]] someone links the same defect report that you did, and claims that since it's a defect report, it has been applied retroactively. 
> > 
> > Apparently [[ https://en.cppreference.com/w/cpp/language/new | cppreference ]] says the same. If you check the defect reports section on the bottom of the page you can see:
> > ```
> > The following behavior-changing defect reports were applied retroactively to previously published C++ standards.
> > 
> > ...
> > DR | Applied to | Behavior as published | Correct behavior
> > CWG 2382 | C++98 | non-allocating placement array new could require allocation overhead | such allocation overhead disallowed
> > ```
> > 
> > So in my understanding you don't need to worry about this overhead at all. I hope this helps.
> Ok it sounds like we shouldn't emit any warnings at all here, unless we're somehow able to get a signal that the user is compiling their code with a pre-2019 compiler that hasn't been updated to reflect this defect report. One way we could get that signal is by putting the checker into the `portability` package which is off-by-default. Then we can explain the situation in the warning message:
> 
> > "Storage provided to placement new is only {0} bytes, which is sufficient to hold the array data but placement new may require additional unspecified array allocation overhead on compilers that don't implement CWG 2382"
> 
> Then if the users enable `portability` but aren't interested in this warning, at least they know how to enable and disable checkers, so they can disable this specific check.
Personally, I think we should definitely not branch on the standard version.
This was never a thing anyway, AFAIK.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129280/new/

https://reviews.llvm.org/D129280