Update to gcc-9-20180729

[belka-ew]

The PR adds new - Walloca-larger-than and -Wno-alloca-larger-than command line switches. -Wno-alloca-larger-than is applied when building druntime since the code like following fails otherwise (rt/arrayassign.d):

void* ptmp = (elementSize > buf.sizeof) ? alloca(elementSize) : buf.ptr;

[iain-buclaw-sociomantic]

Should keep it ASCII collated with the other arguments.

[iain-buclaw-sociomantic]

ASCII collation (move it up with the other Wxxx options).

[iain-buclaw-sociomantic]

Do you have an example to hand of what's warning? Not just a code example, but the warning message given.

Also, how big is too big for alloca?

[belka-ew]

../../../../gcc-9-20180722/libphobos/libdruntime/rt/arrayassign.d: In function ‘_d_arrayassign’:
../../../../gcc-9-20180722/libphobos/libdruntime/rt/arrayassign.d:33:18: error: argument to ‘alloca’ may > be too large [-Werror=alloca-larger-than=]
void* ptmp = (elementSize > buf.sizeof) ? alloca(elementSize) : buf.ptr;
^
../../../../gcc-9-20180722/libphobos/libdruntime/rt/arrayassign.d: In function ‘_d_arraysetassign’:
../../../../gcc-9-20180722/libphobos/libdruntime/rt/arrayassign.d:214:9: error: argument to ‘alloca’ may be too large [-Werror=alloca-larger-than=]
tmp = alloca(element_size)[0 .. element_size];

Also, how big is too big for alloca?

No idea. Anyway, as I see it, druntime should be fixed to handle it properly.
Interesting is the manpage for alloca:

If the allocation causes stack overflow, program behavior is undefined.

So it doesn't just return null if it fails but seems to cause undefined behavior.

By the way, I can't reproduce the segfault on slackware; though I haven't tried ubuntu yet.

[ibuclaw]

What change was made to make the testsuite start passing?

[ibuclaw]

And secondly, rather than adding a new option, can we just change druntime directly?

assert(elementSize < int.max);
void* ptmp = (elementSize > buf.sizeof) ? alloca(cast(int)elementSize) : buf.ptr;

And raise a PR upstream to address it also.

[belka-ew]

What change was made to make the testsuite start passing?

Nothing changed; I couldn't reproduce this locally either.

And secondly, rather than adding a new option, can we just change druntime directly?

We can. Why int.max? <= size_t.max would be identical to the current behaviour.

[ibuclaw]

We can. Why int.max? <= size_t.max would be identical to the current behaviour.

Just an arbitrary restriction really, the important thing to ensure is that can be cast to signed. It looks like the compiler warning default is for any size that may be bigger than half the address space (ptrdiff_t.max would be the real limit).

[belka-ew]

assert doesn't work. only if is apparently recognized.

[belka-ew]

I reverted the makefiles and adjusted the runtime. But should we keep the option itself? alloca is usable from D, is it a good thing to allow the users to ignore the error?

[belka-ew]

I'll raise a PR upstream in the next days.

[ibuclaw]

assert doesn't work. only if is apparently recognized.

The assert is just for the unittest runner. It would be ignored in release build.

Just a cast would be more than enough. It's going to be very unlikely that we have to alloca a data type bigger than ptrdiff_t anyway, let alone 8Mb.

[ibuclaw]

This is the comment in relation to the alloca warning:

Let's not get too specific as to how large the limit
may be.  Someone's clearly an idiot when things
degrade into "if (N > Y) alloca(N)".

[ibuclaw]

FYI: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01805.html

As for the druntime patch, I think there should be a limit applied on the size given to alloca(). If above that size just call malloc() instead with a scope(exit) free(ptr).

[belka-ew]

Let us see what druntime people say. I'll try to prepare a PR during today.

[ibuclaw]

Initially, gcc reviewers seem sceptical of moving alloca related warnings to common, so I guess will have to concede that point and move them here then.

[ibuclaw]

Might as well add all Walloca warning options and documentation.

[ibuclaw]

I think this is fine, just needs the following updates:

1. Add Walloca, Walloca-larger-than= and Wno-alloca-larger-than to language options.
2. Leave out druntime patch for now, on the promise that it will be dealt with upstream soon - it will be merged as soon as its in.

[belka-ew]

Add Walloca, Walloca-larger-than= and Wno-alloca-larger-than to language options.

Do I understand it correctly that Wno-alloca-larger-than shouldn't be explicitly in lang.opt and is supported automatically if Walloca-larger-than= is there?

Leave out druntime patch for now, on the promise that it will be dealt with upstream soon - it will be merged as soon as its in.

And add Wno-alloca-larger-than to the Makefiles again? Otherwise the build fails...

[ibuclaw]

Do I understand it correctly that Wno-alloca-larger-than shouldn't be explicitly in lang.opt and is supported automatically if Walloca-larger-than= is there?

These are all definitions in c.opt, they should be mirrored to make sure that no warnings about unsupported option happens. It also means that each would show up in --help=d too.

Walloca
D
; Documented in C

Walloca-larger-than=
D
; Documented in C

Wno-alloca-larger-than
D
; Documented in C

And add Wno-alloca-larger-than to the Makefiles again? Otherwise the build fails...

But we don't compile with --enable-werror for testing ?

[belka-ew]

These are all definitions in c.opt, they should be mirrored to make sure that no warnings about unsupported option happens. It also means that each would show up in --help=d too.

I used Wno-alloca-larger-than in the druntime Makefiles without defining it in lang.opt.

But we don't compile with --enable-werror for testing ?

Let's see :)

[ibuclaw]

I don't believe it.

libphobos/configure:1426:  --enable-werror         turns on -Werror [default=yes]

It may be wiser to make that default=no as a default.