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

IWYU should not recommend <new> when using size-aware operator delete #777

Closed
adembo opened this issue Mar 22, 2020 · 1 comment
Closed

Comments

@adembo
Copy link

adembo commented Mar 22, 2020

According to https://en.cppreference.com/w/cpp/memory/new/operator_delete, the C++14 size-aware operator delete functions are in the set of functions that are implicitly declared in each translation unit even if the <new> header is not included. And yet, if you use them and run IWYU, it recommends including <new>.

The incorrect recommendation appears to be rooted in iwyu_ast_util.cc's IsDefaultNewOrDelete function, which includes this code:

// Placement-new/delete has 2 args, second is void*.  The only other
// 2-arg overloads of new/delete in <new> take a const nothrow_t&.
if (decl->getNumParams() == 2 &&
    !decl->getParamDecl(1)->getType().isConstQualified())
  return false;

For the size-aware operator delete functions, there are two parameters and the second is a (non-const) size_t, which causes IWYU to return false here and recommend the inclusion of <new>.

My guess is that this function was written pre-C++14 and hasn't been revised since. I've been working on a patch to update it; will submit a separate PR.

adembo added a commit to adembo/include-what-you-use that referenced this issue Mar 23, 2020
…ndards

IsDefaultNewOrDelete attempts to identify when a particular usage of
operator new or operator delete is one of a small set of functions that are
implicitly defined in every translation unit by the compiler. Unfortunately,
it doesn't appear to honor newer implicitly defined variants added in C++14
and C++17.

This patch updates it accordingly, and adds a test that fails without the
patch but passes with it.

Fixes tracking issue include-what-you-use#777.
asfgit pushed a commit to apache/kudu that referenced this issue Mar 27, 2020
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.

Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).

This patch also introduces mappings for libc++. Some are from the IWYU repo
while others are new for Kudu. I also included a patch to fix a bug in IWYU
dealing with false <new> recommendations when processing a codebase using
-fsized-deallocation[1]. Finally, I removed almost all of the "muted" files
from iwyu.py; the few remaining instances trip up IWYU without recourse.

Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.

1. include-what-you-use/include-what-you-use#777

Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Reviewed-on: http://gerrit.cloudera.org:8080/15492
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
Tested-by: Kudu Jenkins
adembo added a commit to adembo/include-what-you-use that referenced this issue Mar 30, 2020
…ndards

IsDefaultNewOrDelete attempts to identify when a particular usage of
operator new or operator delete is one of a small set of functions that are
implicitly defined in every translation unit by the compiler. Unfortunately,
it doesn't appear to honor newer implicitly defined variants added in C++14
and C++17.

This patch updates it accordingly, and adds a test that fails without the
patch but passes with it.

Fixes tracking issue include-what-you-use#777.
adembo added a commit to adembo/include-what-you-use that referenced this issue Apr 1, 2020
…ndards

IsDefaultNewOrDelete attempts to identify when a particular usage of
operator new or operator delete is one of a small set of functions that are
implicitly defined in every translation unit by the compiler. Unfortunately,
it doesn't appear to honor newer implicitly defined variants added in C++14
and C++17.

This patch updates it accordingly, and adds a test that fails without the
patch but passes with it.

Fixes tracking issue include-what-you-use#777.
kimgr added a commit to kimgr/include-what-you-use that referenced this issue Apr 12, 2020
This replaces builtins_new_included.cc with operator_new.cc, and covers
all variants of operator new that do _not_ require <new> to be
included:

- Raw explicit ::operator new/::operator delete calls
- New expressions, both of builtin and user-defined types
- Aligned allocation (currently disabled, as we misdiagnose it,
  see PR include-what-you-use#777).
kimgr pushed a commit to kimgr/include-what-you-use that referenced this issue Apr 12, 2020
…ndards

IsDefaultNewOrDelete attempts to identify when a particular usage of
operator new or operator delete is one of a small set of functions that are
implicitly defined in every translation unit by the compiler. Unfortunately,
it doesn't appear to honor newer implicitly defined variants added in C++14
and C++17.

This patch updates it accordingly, and adds a test that fails without the
patch but passes with it.

Fixes tracking issue include-what-you-use#777.
kimgr added a commit to kimgr/include-what-you-use that referenced this issue Apr 13, 2020
This replaces builtins_new_included.cc with operator_new.cc, and covers
all variants of operator new that do _not_ require <new> to be
included:

- Raw explicit ::operator new/::operator delete calls
- New expressions, both of builtin and user-defined types
- Aligned allocation (currently disabled, as we misdiagnose it,
  see PR include-what-you-use#777).
kimgr pushed a commit to kimgr/include-what-you-use that referenced this issue Apr 13, 2020
…ndards

IsDefaultNewOrDelete attempts to identify when a particular usage of
operator new or operator delete is one of a small set of functions that are
implicitly defined in every translation unit by the compiler. Unfortunately,
it doesn't appear to honor newer implicitly defined variants added in C++14
and C++17.

This patch updates it accordingly, and adds a test that fails without the
patch but passes with it.

Fixes tracking issue include-what-you-use#777.
kimgr added a commit to kimgr/include-what-you-use that referenced this issue Apr 13, 2020
Instead of 'IsDefaultNewOrDelete' which uses heuristics and string
parsing, use FunctionDecl::isReplaceableGlobalAllocationFunction from
the Clang API. It returns true for any replaceable allocation function,
which happens to coincide nicely with all allocation functions except
placement new.

Fixes include-what-you-use#777 and improves behavior for the modern allocation functions:

- Sized deallocation in C++14
- Aligned allocation in C++17

This small improvement fixes a number of TODOs in the new placement_new
testcase, and makes it possible to add a testcase for implicit aligned
allocation in operator_new, as aligned allocation no longer requires
<new>.

Patch based on work and lots of good input from Adar Dembo.
@kimgr
Copy link
Contributor

kimgr commented Apr 14, 2020

We did lots of research in the context of PR #778, and learned the following:

  • new and delete have two family trees: one as overloadable operators, and one as expressions, whose semantics are built into the language.
  • the semantics of new X; is to first call operator new for X, whether built-in or user-defined, to allocate memory, and then execute X's constructor.
  • operator new(size_t) is declared in <new>, but its declaration is made implicitly available by the compiler, as well as a definition, if none is provided by the user. This is called a 'replaceable allocation function'.
  • operator new(size_t, void*) is the non-allocating allocation function for placement syntax, i.e. new (<some pointer>) X; -- again, it calls the (noop) allocation function, and then executes X's constructor.
  • operator new(size_t, void*) is defined in <new>, and so can't be replaced/overloaded.
  • There are also placement-like operators provided to enable syntax new (std::nothrow) X; and new (std::align_val_t(32)) X;, but these are replaceable (so that the user can provide custom allocation with alignment no-throw guarantees)
  • Finally, while you can't replace the global placement allocation function, you can overload it for more specific types, e.g. void* operator new(size_t sz, Arena& arena);. Used like this new (arena) X;, it could delegate allocation to an arena that's more efficient than built-in malloc.
  • The compiler is really helpful with the replaceable functions; their declarations are implicitly available and a definition is generated unless one is already provided by the user
  • One exception to the above bullet is std::nothrow and std::align_val_t -- these two symbols are not made implicitly available, you need to explicitly #include <new> to use them (there is a special case for C++17 where the std::align_val_t overload is implicitly used if the type being new-ed has alignment restrictions. No include necessary in that case.)
  • The other exception is ::operator new(size_t, void*) -- as it is both trivial and not replaceable, the standard library forces one definition into the translation unit when <new> is included.

Note that it is possible to define your own ::operator new(size_t, void*), so in some sense it is replaceable in practice.

All this said, for IWYU there's a few simple rules:

  • If the user employs normal placement new -- ::operator new(size_t, void*), using e.g. char buf[128]; new(buf) X; -- they should include <new> to get the definition in place. Because no other symbols indicate where the declaration should come from, we need a special rule in IWYU to recognize placement new and map it to <new>.
  • If the user employs no-throw or aligned new (which kind of look like placement), they spell std:: symbols, which will lead IWYU to <new> by its normal logic. No special handling necessary.
  • All other allocation functions and new-expressions are handled implicitly by the compiler, and require no additional includes.

This is reflected in PR #784.

kimgr added a commit to kimgr/include-what-you-use that referenced this issue May 2, 2020
Instead of 'IsDefaultNewOrDelete' which uses heuristics and string
parsing, use FunctionDecl::isReplaceableGlobalAllocationFunction from
the Clang API. It returns true for any replaceable allocation function,
which happens to coincide nicely with all allocation functions except
placement new.

Fixes include-what-you-use#777 and improves behavior for the modern allocation functions:

- Sized deallocation in C++14
- Aligned allocation in C++17

This small improvement fixes a number of TODOs in the new placement_new
testcase, and makes it possible to add a testcase for implicit aligned
allocation in operator_new, as aligned allocation no longer requires
<new>.

Patch based on work and lots of good input from Adar Dembo.
@kimgr kimgr closed this as completed in a85cea1 May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants