Bug 68680 - [4.9 regression] On-stack VLA does not cause instrumentation with -fstack-protector
Summary: [4.9 regression] On-stack VLA does not cause instrumentation with -fstack-pro...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 5.1.1
: P3 normal
Target Milestone: 4.9.4
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-03 16:08 UTC by Florian Weimer
Modified: 2016-02-11 09:36 UTC (History)
3 users (show)

See Also:
Host:
Target: x864_64-redhat-linux-gnu
Build:
Known to work: 4.6.4, 6.0
Known to fail: 4.7.0, 5.1.0, 5.3.0
Last reconfirmed: 2015-12-03 00:00:00


Attachments
gcc6-pr68680.patch (636 bytes, patch)
2015-12-03 18:32 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Weimer 2015-12-03 16:08:55 UTC
This test case

int process(char *);

int
uses_vla(unsigned long sz)
{
  char buf[sz];
  return process(buf);
}

compiles to:

uses_vla:
	pushq	%rbp
	addq	$15, %rdi
	andq	$-16, %rdi
	movq	%rsp, %rbp
	subq	%rdi, %rsp
	movq	%rsp, %rdi
	call	process
	leave
	ret

This happens with the C and C++ compilers.

The process function may have a buffer overflow, so stack protector instrumentation is required here.

The equivalent test case with alloca passes.
Comment 1 Martin Sebor 2015-12-03 17:36:29 UTC
Confirmed with the top of trunk.  GCC 5.1.0 behaves as expected.
Comment 2 Jakub Jelinek 2015-12-03 17:56:40 UTC
Weird, I think this issue has been introduced with r179655, i.e. 5.1 nor 4.9 works.  They work only with -O0.
Anyway, I think I see the problem.
Comment 3 Jakub Jelinek 2015-12-03 18:32:24 UTC
Created attachment 36903 [details]
gcc6-pr68680.patch

Seems the problem is that with the addition of __builtin_alloca_with_align, which is now used for VLAs, the code has not been adjusted to make sure that cfun->calls_alloca is set already during the GIMPLE optimizers, so it will be set only during expansion, when expanding that builtin (which calls allocate_dynamic_stack_space).  There are a few spots that care about cfun->calls_alloca even before that:
1) stack protector (is testing this at the start of expansion, so before the builtin has been expanded)
2) tailcall optimization (I'd expect we want to continue what 4.6 and older did,
forbid tail calls if there are even VLAs, but am not 100% sure)
3) the inliner (again, not 100% sure we want to handle VLAs as alloca, but maybe we should)
4) some missed optimizations (tests whether result of the __builtin_alloca_with_align will be always non-NULL)
The patch just arranges for the VLAs to be treated like alloca for these purposes.  If that is not what we want now in some of the cases - 1) surely wants to handle them as 4.6 did - then perhaps we need two separate ECF_ flags and two separate cfun-> bit fields.
Comment 4 Richard Biener 2015-12-04 10:01:39 UTC
As -fstack-protector-strong is new with GCC 4.9 I don't see how this is a regression?  -fstack-protector-all works AFAICS.
Comment 5 Jakub Jelinek 2015-12-04 10:50:27 UTC
Sure, but try just normal -fstack-protector, the regression is with that option.
Comment 6 Richard Biener 2015-12-04 12:21:31 UTC
Ah, ok.  4.6 works.
Comment 7 Jakub Jelinek 2015-12-04 16:32:54 UTC
Author: jakub
Date: Fri Dec  4 16:32:22 2015
New Revision: 231279

URL: https://gcc.gnu.org/viewcvs?rev=231279&root=gcc&view=rev
Log:
	PR tree-optimization/68680
	* calls.c (special_function_p): Return ECF_MAY_BE_ALLOCA for
	BUILT_IN_ALLOCA{,_WITH_ALIGN}.  Don't check for __builtin_alloca
	by name.

	* gcc.target/i386/pr68680.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr68680.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
    trunk/gcc/testsuite/ChangeLog
Comment 8 Jakub Jelinek 2015-12-04 18:07:02 UTC
Fixed for 6+, testing 5.3 backport now.
Comment 9 Jakub Jelinek 2015-12-07 09:11:38 UTC
Author: jakub
Date: Mon Dec  7 09:11:06 2015
New Revision: 231357

URL: https://gcc.gnu.org/viewcvs?rev=231357&root=gcc&view=rev
Log:
	Backport from mainline
	2015-12-04  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/68680
	* calls.c (special_function_p): Return ECF_MAY_BE_ALLOCA for
	BUILT_IN_ALLOCA{,_WITH_ALIGN}.  Don't check for __builtin_alloca
	by name.

	* gcc.target/i386/pr68680.c: New test.

Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.target/i386/pr68680.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/calls.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 10 Jakub Jelinek 2016-02-11 09:10:30 UTC
Author: jakub
Date: Thu Feb 11 09:09:58 2016
New Revision: 233322

URL: https://gcc.gnu.org/viewcvs?rev=233322&root=gcc&view=rev
Log:
	Backported from mainline
	2015-12-04  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/68680
	* calls.c (special_function_p): Return ECF_MAY_BE_ALLOCA for
	BUILT_IN_ALLOCA{,_WITH_ALIGN}.  Don't check for __builtin_alloca
	by name.

	* gcc.target/i386/pr68680.c: New test.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/i386/pr68680.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/calls.c
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog
Comment 11 Jakub Jelinek 2016-02-11 09:36:10 UTC
Fixed.