Bug 85783 - alloc-size-larger-than fires incorrectly with new[] and can't be disabled
Summary: alloc-size-larger-than fires incorrectly with new[] and can't be disabled
Status: RESOLVED WONTFIX
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.1.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2018-05-14 21:20 UTC by Paul Smith
Modified: 2018-05-17 15:00 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-05-16 00:00:00


Attachments
sample source file (549 bytes, text/x-csrc)
2018-05-14 21:20 UTC, Paul Smith
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Smith 2018-05-14 21:20:44 UTC
Created attachment 44131 [details]
sample source file

GCC 8.1.0 / binutils 2.30
GNU/Linux x86_64 with a sysroot of Red Hat EL 6.5.

Recently I started upgrading from GCC 7.3 to GCC 8.1.  I discovered three locations in my codebase where the alloc-size-larger-than warning is generated.  It wasn't ever generated with 7.3.  Since I build with -Wall -Werror this causes compiles to fail.

The first issue is, I wasn't able to find any way to turn off this warning other than by removing -Wall which seems entirely too severe.  There should be some way to disable it; maybe by providing -Walloc-size-larger-than=0 or similar.

I did work around this issue by casting the value given to new[] to type unsigned int, but that's unpleasant.

Of course removing the false positive would be helpful as well.

I spent quite a while trying to create a small sample; the results are below.  Most any change to this file appears to cause the warning to go away: for example I tried to use a simple template I created rather than std::shared_ptr<>, or even remove that field altogether: no warning.  If I remove or modify the if-statements in the method significantly, no warning.  Etc.  I didn't try all changes of course.  Also without optimization it doesn't warn but with both -O1 and -O2 it does.

Results of compiling the attached file:

  $ x86_64-rh65-linux-gnu-g++ -v -o /tmp/SP.o -c -O2 -Wall -Werror params.cpp
  Using built-in specs.
  COLLECT_GCC=/work/src/build/x86_64-linux/cc/generic/bin/x86_64-generic-linux-gnu-g++
  Target: x86_64-generic-linux-gnu
  Configured with: /work/src/cc/gcc-8.1.0/configure --disable-nls --disable-werror --prefix=/work/src/cc/x86_64-linux/final/generic --host=x86_64-tools-linux-gnu --target=x86_64-generic-linux-gnu --with-sysroot=/work/src/build/x86_64-linux/sysroot/generic CFLAGS=-O2 CXXFLAGS=-O2 LDFLAGS=-O2 --enable-gold --enable-languages=c,c++
  Thread model: posix
  gcc version 8.1.0 (GCC) 
  COLLECT_GCC_OPTIONS='-m64' '-isystem' '=/usr/include-fixed' '-v' '-o' '/tmp/SP.o' '-c' '-O2' '-Wall' '-Werror' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
  /work/src/build/x86_64-linux/cc/generic/bin/../libexec/gcc/x86_64-generic-linux-gnu/8.1.0/cc1plus -quiet -v -iprefix /work/src/build/x86_64-linux/cc/generic/bin/../lib/gcc/x86_64-generic-linux-gnu/8.1.0/ -isysroot /work/src/build/x86_64-linux/sysroot/rh65 -D_GNU_SOURCE -isystem =/usr/include-fixed params.cpp -quiet -dumpbase SortParameters.cpp -m64 -mtune=generic -march=x86-64 -auxbase-strip /tmp/SP.o -O2 -Wall -Werror -version -o /tmp/ccuJobAh.s
  GNU C++14 (GCC) version 8.1.0 (x86_64-generic-linux-gnu)
        compiled by GNU C version 8.1.0, GMP version 6.1.0, MPFR version 3.1.4, MPC version 1.0.3, isl version isl-0.18-GMP

  GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
  Compiler executable checksum: e2fe942476766bd673c0e36030131141
  In function 'void* operator new [](size_t)',
    inlined from 'static Params* Params::buildParams(size_t, Info*)' at params.cpp:52:47:
  params.cpp:8:21: error: argument 1 value '18446744073709551615' exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
       void* p = malloc(size);
                 ~~~~~~^~~~~~
    ...
  /work/src/build/x86_64-linux/sysroot/rh65/usr/include/stdlib.h: In static member function 'static Params* Params::buildParams(size_t, Info*)':
  /work/src/build/x86_64-linux/sysroot/rh65/usr/include/stdlib.h:471:14: note: in a call to allocation function 'void* malloc(size_t)' declared here
   extern void *malloc (size_t __size) __THROW __attribute_malloc__ __wur;
                ^~~~~~
  cc1plus: all warnings being treated as errors
Comment 1 Andrew Pinski 2018-05-14 21:35:17 UTC
Did you try:
-Wno-alloc-size-larger-than ?

Also in your code, numberFields is a signed int and then casted to size_t.  On LP64 targets (or rather sizeof(size_t) != sizeof(int)), then value is sign extended.
Comment 2 Paul Smith 2018-05-14 21:51:24 UTC
> Did you try: -Wno-alloc-size-larger-than ?

Yes.

  cc1plus: error: unrecognized command line option '-Wno-alloc-size-larger-than'

> Also in your code, numberFields is a signed int and then casted to size_t.  On LP64 targets (or rather sizeof(size_t) != sizeof(int)), then value is sign extended.

I'm not suggesting the code is well-written.  Just that it shouldn't throw a warning.

In any event, if I change numberFields to "unsigned int" I still get the same warning.
Comment 3 Martin Sebor 2018-05-15 01:21:23 UTC
I can reproduce it with the following reduced semi-valid test case.  (The test case isn't entirely valid because it defines the array new to exit on failure rather than to throw, but a nothrow form of the test case reproduces the warning as well.)

void* operator new[](__SIZE_TYPE__ n)
{
  void* p = __builtin_malloc (n);
  if (p)
    return p;

  __builtin_exit (1);
}

struct A
{
  A ();
  ~A ();
};


void* f (__SIZE_TYPE__ n)
{
  if (!n)
    return 0;

  return new A[n];
}
In function ‘void* operator new [](long unsigned int)’,
    inlined from ‘void* f(long unsigned int)’ at pr85783.C:22:17:
pr85783.C:3:30: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
   void* p = __builtin_malloc (n);
             ~~~~~~~~~~~~~~~~~^~~
pr85783.C: In function ‘void* f(long unsigned int)’:
pr85783.C:3:30: note: in a call to built-in allocation function ‘void* __builtin_malloc(long unsigned int)’

The excessive constant argument is introduced by the C++ front-end, in cp/build_operator_new_call(), which emits a call to operator new[](size_t).  The code was added in r190546 as a solution to prevent unsigned wrapping (when array new expression must compute the amount of space to allocate as a product of the number of elements and element size).  When the replacement operator new[] is inlined the excessive argument is propagated to malloc() and ultimately triggers the warning.

An easy way to suppress the warning is to check the argument in the operator and have it fail when it's excessive:

  void* operator new[](size_t n)
  {
    if (n <= PTRDIFF_MAX)
      {
        void* p = malloc (n);
        if (p)
          return p;
      }

    exit (1);
  }

Otherwise, I'm not sure there is much GCC can do to avoid the warning, or that it's worth putting effort into.  The use case (defining a non-throwing replacement operator new inline) is rare and not entirely valid.  Normally, the operator will be defined out-of-line and while that might still trigger warnings from the same translation unit, the solution -- handling the excessive argument in the replacement operator -- seems simple enough.

With that I'm going to resolve this report as WONTFIX.  If someone (e.g., Jeff who chimed in on the discussion on gcc-help below) feels otherwise please feel free to reopen it.

https://gcc.gnu.org/ml/gcc-help/2018-05/msg00088.html
Comment 4 Paul Smith 2018-05-15 05:34:38 UTC
Well, clearly I disagree with this conclusion and feel this is a bug.

At the very least, the fact that it's impossible to disable the warning needs to be considered a bug.  The statement on the mailing list from Martin Sebor was:

> -Walloc-size-larger-than, like most (all?) other middle-end
> warnings, is designed to trigger only for calls that truly have
> an argument in excess of the limit.  Unlike -Wmaybe-uninitialized,
> it is not intended to diagnose case where the argument may but
> need not be excessive (i.e., it's not expected to have false
> positives, and I don't think it is particularly prone to them).

If this WONTFIX resolution means that we are not going to fix known false positives for this warning, then IMO the above description of the situation is not accurate and we should add the ability to disable the warning.
Comment 5 Paul Smith 2018-05-15 12:50:14 UTC
I simplified my example too much; I think this should be re-opened.

In my real code, operator new[] does not invoke exit(); it invokes my own function (which is defined as noreturn, but that's not required).  There's no way for the compiler to know whether this function will throw or not, so "replacing a non-throwing operator new[]" isn't why my case is not working.  Also, you mention an inline implementation of operator new[], but there's nothing in your code example that I can see that forces my replacement to be inline.  Is there some magic about global operator new replacement that I'm forgetting?

Here's an example which still fails with the warning and which I think is valid C++ (interestingly this version requires -O2 to show the problem):

void allocFail(__SIZE_TYPE__ _s);

void* operator new[](__SIZE_TYPE__ n)
{
  void* p = __builtin_malloc (n);
  if (!p)  allocFail (n);
  return p;
}

struct A
{
  A ();
  ~A ();
};


void* f (__SIZE_TYPE__ n)
{
  if (!n)
    return 0;

  return new A[n];
}

In function 'void* operator new [](long unsigned int)',
    inlined from 'void* f(long unsigned int)' at p1.cpp:22:17:
p1.cpp:5:30: warning: argument 1 value '18446744073709551615' exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
   void* p = __builtin_malloc (n);
             ~~~~~~~~~~~~~~~~~^~~
p1.cpp: In function 'void* f(long unsigned int)':
p1.cpp:5:30: note: in a call to built-in allocation function 'void* __builtin_malloc(long unsigned int)'
Comment 6 Martin Sebor 2018-05-15 16:41:14 UTC
Inlining happens automatically unless it's disabled.  The inlining of the operator results in the excessive constant argument (created by the C++ front-end) to propagate to the call to malloc().  That's what triggers the warning.  The GIMPLE for the function in my smaller test case (from the output of -fdump-tree-gimple) shows this:

f (size_t n)
{
  void * D.6084;
  void * D.6059;
  size_t iftmp.1;
  long unsigned int n.2;
  struct A * retval.3;
  struct A * D.6060;
  struct A * D.6061;
  long int D.6062;
  size_t iftmp.4;

  if (n == 0) goto <D.6082>; else goto <D.6083>;
  <D.6082>:
  D.6084 = 0B;
  // predicted unlikely by early return (on trees) predictor.
  return D.6084;
  <D.6083>:
  n.2 = n;
  if (n.2 <= 9223372036854775800) goto <D.6087>; else goto <D.6088>;
  <D.6087>:
  iftmp.1 = n.2 + 8;
  goto <D.6089>;
  <D.6088>:
  iftmp.1 = 18446744073709551615;       <<< SIZE_MAX
  <D.6089>:
  D.6059 = operator new [] (iftmp.1);   <<< call to replacement operator new
  MEM[(sizetype *)D.6059] = n.2;

(Adding attribute noinline to the operator eliminates the warning.)

As we discussed in the past, the C++ standard imposes a couple of requirements on replacement operator new that your test case violates:

1) it must not be declared inline (there must be exactly one operator new in a program)
2) it must either return valid new pointer or fail by throwing an exception

To make the test case strictly valid the replacement operator would either have to throw, or it would have to be the nothrow form (taking const std::nothrow_t& as an argument), and the call to it would have to pass it std::nothrow.

The warning doesn't trigger if the replacement operator throws.  It does trigger for the nothrow operator, which is why I agree the warning in that case is a false positive.  (But you're not replacing the nothrow operator.)

Having said that, the C++ standard requires compilers to avoid calling the allocation function with an argument in excess of some implementation-defined limit, but GCC calls it regardless (e.g., it calls it with N = SIZE_MAX for new (nothrow) T[N] even when sizeof (T) > 1).  That's a bug, and (indirectly) the reason for the warning.  I've raised bug 85795 for that.
Comment 7 Paul Smith 2018-05-15 17:01:53 UTC
Is there a way (in standard C++) to force non-inline?  I'm not aware of one.  So that means the only standard-conforming way to replace operator new is if it's in its own compilation unit all by itself?  I don't have a copy of the standard but cppreference says only that replacement operator new can't have an inline specifier, that it can't be static, and that it has to be in the global namespace, all of which requirements this example appears to meet.

Regarding throw, does the standard really say that the throw must be explicit in the implementation of the function directly?  If my operator new[] invokes a function to throw, rather than throwing directly, is that not standard-conforming?  That seems bizarre to me: just because the compiler can't prove to itself that my operator new will throw properly, the compiler is allowed to assume the code is non-conforming?
Comment 8 Martin Sebor 2018-05-15 17:29:56 UTC
There is no standard way to say "don't inline this function."  It's up to the compiler to decide what to inline just so long as it doesn't violate the requirements.

There is also no requirement that the definition of the replacement operator contain an explicit throw statement.  Your example in comment #5 is fine.

But I'm not sure what the point is of these questions.  I explained what causes the warning, and what makes the original test case less than 100% conforming.  I'm not disputing that the warning is unhelpful (with a conforming test case or otherwise), or that it would be nice to avoid.  I just don't see a way to do it in GCC (except by fixing bug 85795).  That's why I resolved this as WONTFIX and not as INVALID, and why I opened bug 85795.  That's a separate conformance issue that should be considered on its own.  If that happens to get rid of the warning that's a bonus.

What would you prefer me to do instead?
Comment 9 Paul Smith 2018-05-15 17:48:32 UTC
Sorry; Andrew's original reply seemed to say that the use-case is non-conforming so the issue was WONTFIX.  I also thought your comment #6 was referring to my example in comment #5: I just wanted to clarify that that example was conforming (although still, admittedly, very unrealistic).

If this issue is best left WONTFIX in deference to the more accurate/detailed one you opened that's fine with me.

Cheers!
Comment 10 Alexander Monakov 2018-05-16 09:58:42 UTC
Reopening: the request to be able to disable the warning (via -Wno-alloc-size-larger-than) is valid and should be addressed.
Comment 11 Eric Gallager 2018-05-16 20:14:05 UTC
(In reply to Alexander Monakov from comment #10)
> Reopening: the request to be able to disable the warning (via
> -Wno-alloc-size-larger-than) is valid and should be addressed.

I think that part is bug 82063
Comment 12 Martin Sebor 2018-05-17 15:00:20 UTC
That's right, bug 82063 tracks the inability to turn off the warning.  This report is about the false positive (only).  Hopefully, that will go away once we fix 85795.