Bug 55513 - [4.7/4.8 Regression] Incorrect snprintf folding when building with -std=c++0x
Summary: [4.7/4.8 Regression] Incorrect snprintf folding when building with -std=c++0x
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.7.2
: P2 normal
Target Milestone: 4.7.3
Assignee: Aldy Hernandez
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2012-11-28 10:03 UTC by Amit Schreiber
Modified: 2012-12-12 09:57 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.6.3
Known to fail: 4.7.0, 4.7.1, 4.7.2, 4.8.0
Last reconfirmed: 2012-11-28 00:00:00


Attachments
The program (209 bytes, text/x-c++src)
2012-11-28 10:05 UTC, Amit Schreiber
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Amit Schreiber 2012-11-28 10:03:54 UTC
Using the following program:

  #include <cstdio>
  #include <iostream>

  int main() {
    char str[10];
    const int strLength = snprintf(str, sizeof(str), "Hello");
    std::cout << strLength << ':' << str << std::endl;
    return 0;
  }

When built with the following command:
g++ test_snprintf.cc -O0 -g3 -std=c++0x

The output of running it is:
5: 
instead of:
5: Hello

The program will work fine if either of the following changes are made
1. -std=c++0x is removed from the build command line.
2. The const is removed from the definition of the variable strLength

Here's a disassembly of main() up until the call to ostream::operator<<, when built with -std=c++0x:
   0x0000000000400a6c <+0>:     push   %rbp
   0x0000000000400a6d <+1>:     mov    %rsp,%rbp
   0x0000000000400a70 <+4>:     sub    $0x30,%rsp
   0x0000000000400a74 <+8>:     mov    %fs:0x28,%rax
   0x0000000000400a7d <+17>:    mov    %rax,-0x8(%rbp)
   0x0000000000400a81 <+21>:    xor    %eax,%eax
   0x0000000000400a83 <+23>:    movl   $0x5,-0x24(%rbp)
   0x0000000000400a8a <+30>:    mov    $0x5,%esi
   0x0000000000400a8f <+35>:    mov    $0x6010a0,%edi
   0x0000000000400a94 <+40>:    callq  0x4008a0 <_ZNSolsEi@plt>
   0x0000000000400a99 <+45>:    mov    $0x3a,%esi
   0x0000000000400a9e <+50>:    mov    %rax,%rdi
   0x0000000000400aa1 <+53>:    callq  0x4008e0 <_ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_c@plt>

Here's a disassembly of main() up until the call to ostream::operator<<, when built without -std=c++0x _or_ when the const is removed (same result):
   0x0000000000400a6c <+0>:     push   %rbp
   0x0000000000400a6d <+1>:     mov    %rsp,%rbp
   0x0000000000400a70 <+4>:     sub    $0x30,%rsp
   0x0000000000400a74 <+8>:     mov    %fs:0x28,%rax
   0x0000000000400a7d <+17>:    mov    %rax,-0x8(%rbp)
   0x0000000000400a81 <+21>:    xor    %eax,%eax
   0x0000000000400a83 <+23>:    lea    -0x20(%rbp),%rax
   0x0000000000400a87 <+27>:    movl   $0x6c6c6548,(%rax)
   0x0000000000400a8d <+33>:    movw   $0x6f,0x4(%rax)
   0x0000000000400a93 <+39>:    movl   $0x5,-0x24(%rbp)
   0x0000000000400a9a <+46>:    mov    -0x24(%rbp),%eax
   0x0000000000400a9d <+49>:    mov    %eax,%esi
   0x0000000000400a9f <+51>:    mov    $0x6010a0,%edi
   0x0000000000400aa4 <+56>:    callq  0x4008a0 <_ZNSolsEi@plt>
   0x0000000000400aa9 <+61>:    mov    $0x3a,%esi
   0x0000000000400aae <+66>:    mov    %rax,%rdi
   0x0000000000400ab1 <+69>:    callq  0x4008e0 <_ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_c@plt>


OS Version: Ubuntu 12.10

uname -a output:
Linux desktop 3.5.0-18-generic #29-Ubuntu SMP Fri Oct 19 10:26:51 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux

gcc --version output:
gcc (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2
Comment 1 Amit Schreiber 2012-11-28 10:05:16 UTC
Created attachment 28819 [details]
The program
Comment 2 Jonathan Wakely 2012-11-28 10:18:57 UTC
Confirmed. -fno-builtin fixes it too.
Comment 3 Richard Biener 2012-11-28 12:35:16 UTC
Seems to be simply folded away.
Comment 4 Jakub Jelinek 2012-11-28 14:25:44 UTC
With const int strLength = snprintf(str, 10, "Hello"); instead it fails also on the trunk, with sizeof(str) it has been "fixed" by my
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192141
changes.
Comment 5 Jakub Jelinek 2012-11-28 15:15:11 UTC
In cp_parser_constant_expression when we call cp_parser_assignment_expression
on the initializer, it returns
((char *) __builtin_memcpy ((char *) &str, (const char *) "Hello", 6);, 5)
As this is C++11, we call potential_rvalue_constant_expression which returns true (the call in the COMPOUND_EXPR is builtin, and all its arguments are pontential_rvalue_constant_expression arguments).  I bet somewhere later on we see that it is const and ignore the TREE_SIDE_EFFECTS initializer.
Comment 6 Jakub Jelinek 2012-11-28 15:51:21 UTC
Reduced testcase:
void
f1 ()
{
  char s[10];
  const int t = __builtin_snprintf (s, 10, "Hello");
  __builtin_printf ("%d %s\n", t, s);
}

void
f2 ()
{
  char s[10];
  const int t = (__builtin_memcpy (s, "Hello", 6), 5);
  __builtin_printf ("%d %s\n", t, s);
}

int
main ()
{
  f1 ();
  f2 ();
}

Even f2 fails to call memcpy, so it isn't related to folding the snprintf into what it folds to, but about handling COMPOUND_EXPRs with side-effects on the LHS where the only side-effects are builtin calls.  If the LHS of the COMPOUND_EXPR is a non-builtin, potential_rvalue_constant_expression returns false and it is handled correctly, but even if it is say __builtin_exit (0) or similar, it is optimized away.
Comment 7 Aldy Hernandez 2012-12-05 17:08:48 UTC
I'll take a look.
Comment 8 Aldy Hernandez 2012-12-06 21:06:29 UTC
After Jason's patch here (trunk@177066):

        PR c++/49813
        * semantics.c (potential_constant_expression_1): Allow any builtin.
        (morally_constexpr_builtin_function_p): Remove.

It seems we return true for any built-in in potential_constant_expression_1.  This means that check_initializer->store_init_value->maybe_constant_init will chop off the built-in side of a COMPOUND_EXPR when setting a DECL_INITIAL, transforming:

(COMPOUND_EXPR (CALL_EXPR built-in) INTEGER_CST)

into

INTEGER_CST

Since potential_constant_expression_1 returns true for any built-in, we end up removing even a __builtin_exit from code like this:

const int t = (__builtin_exit(0), 777);

Jason, is this by design? Should we not limit the conditional here further?:

                   /* Allow any built-in function; if the expansion
                      isn't constant, we'll deal with that then.  */
                   && !is_builtin_fn (fun))
Comment 9 Jason Merrill 2012-12-07 05:21:01 UTC
(In reply to comment #8)
>                    /* Allow any built-in function; if the expansion
>                       isn't constant, we'll deal with that then.  */
>                    && !is_builtin_fn (fun))

The problem seems to be in the dealing with it "then"; cxx_eval_builtin_function_call needs to actually check whether folding the call produces a constant or not.
Comment 10 Aldy Hernandez 2012-12-08 01:20:33 UTC
Author: aldyh
Date: Sat Dec  8 01:19:13 2012
New Revision: 194316

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194316
Log:
	PR c++/55513
	* semantics.c (cxx_eval_builtin_function_call): Set non_constant_p
	after folding.

Added:
    trunk/gcc/testsuite/g++.dg/pr55513.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/semantics.c
Comment 11 Aldy Hernandez 2012-12-08 01:24:30 UTC
Fixed in trunk.
Comment 12 Pawel Sikora 2012-12-12 09:57:32 UTC
(In reply to comment #11)
> Fixed in trunk.

no backport to 4.7 branch?