Bug 79993 - [6 Regression] ICE in tree_to_uhwi, at tree.c:7344
Summary: [6 Regression] ICE in tree_to_uhwi, at tree.c:7344
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 7.0
: P2 normal
Target Milestone: 7.0
Assignee: Jason Merrill
URL:
Keywords: ice-on-valid-code
: 80269 (view as bug list)
Depends on:
Blocks: 83808
  Show dependency treegraph
 
Reported: 2017-03-10 20:17 UTC by Martin Liška
Modified: 2018-10-26 11:45 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-03-17 00:00:00


Attachments
gcc7-pr79993.patch (2.10 KB, patch)
2017-03-29 13:31 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2017-03-10 20:17:06 UTC
Starting from 4.9 we ICE on:

./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/opt/pr78201.C -c -fsanitize=address
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/opt/pr78201.C:13:1: internal compiler error: in tree_to_uhwi, at tree.c:7344
 }
 ^
0x150f970 tree_to_uhwi(tree_node const*)
	../../gcc/tree.c:7344
0x11c1818 asan_add_global
	../../gcc/asan.c:2422
0x11c56ee add_string_csts(constant_descriptor_tree**, asan_add_string_csts_data*)
	../../gcc/asan.c:2637
0x11c7f26 void hash_table<tree_descriptor_hasher, xcallocator>::traverse<asan_add_string_csts_data*, &(add_string_csts(constant_descriptor_tree**, asan_add_string_csts_data*))>(asan_add_string_csts_data*)
	../../gcc/hash-table.h:987
0x11c5a7d asan_finish_file()
	../../gcc/asan.c:2709
Comment 1 Marek Polacek 2017-03-17 16:43:49 UTC
Confirmed.
Comment 2 Marek Polacek 2017-03-17 16:51:49 UTC
Started with

commit d680c844a90ba12a0b12f7d206d697dc32d2cfee
Author: jason <jason@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu May 9 16:43:43 2013 +0000

            N3639 C++1y VLA diagnostics
    
            * decl.c (grokdeclarator): Complain about reference, pointer, or
            typedef to VLA.
            (create_array_type_for_decl): Complain about array of VLA.
            * pt.c (tsubst): Likewise.
            * rtti.c (get_tinfo_decl): Talk about "array of runtime bound".
            * semantics.c (finish_decltype_type): Complain about decltype of VLA.
            * typeck.c (cp_build_addr_expr_1): Complain about VLA.
            (cxx_sizeof_or_alignof_type): Likewise.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@198746 138bc75d-0d04-0410-961f-82ee72b054a4

before that:
pr78201.C:11:15: error: variable-sized object ‘a’ may not be initialized
   char a[e] = "";
               ^
Comment 3 Jakub Jelinek 2017-03-21 17:15:22 UTC
I think we have various dups of this, the FE emits STRING_CST with VLA array type which is invalid.
This happens in digest_init_r, which for init being STRING_CST does:
          if (type != TREE_TYPE (init))
            {
              init = copy_node (init);
              TREE_TYPE (init) = type;
            }
I think if type is VLA, we should just keep the STRING_CST as is and do something like __builtin_memcpy from the STRING_CST to the VLA, maybe followed by __builtin_memset for the remainder if the STRING_CST is shorter than the VLA size.

I can surely work around this in asan.c by ignoring STRING_CSTs with non-constant sizes, but that just seems to be papering over the real bug.
Comment 4 Jakub Jelinek 2017-03-29 13:31:31 UTC
Created attachment 41072 [details]
gcc7-pr79993.patch

So, one option is to revert to the 4.8 and earlier behavior, disallow any VLA initialization (like C does).  This patch should do it.

Otherwise, the behavior the C++ FE has when not using string literals as initializers is that it is UB if the VLA is smaller than the size of the initializer, and if it is larger or equal than that, it is initialized from the initializer and excess elements if any are zero initialized (value initialization or whatever it is).  Even when ignoring the bogus type on the STRING_CST, we don't implement that right now for STRING_CST - we probably want memcpy from the STRING_CST followed by whatever we do for other initializers.
Comment 5 Jason Merrill 2017-03-29 20:45:01 UTC
(In reply to Jakub Jelinek from comment #4)
> Created attachment 41072 [details]
> gcc7-pr79993.patch
> 
> So, one option is to revert to the 4.8 and earlier behavior, disallow any
> VLA initialization (like C does).  This patch should do it.

I've been ambivalent about this.  I think it makes sense, but given the number of testcases that hit the change, the functionality seems to be fairly popular.
Comment 6 Martin Sebor 2017-03-29 21:04:21 UTC
I think it would be preferable to make VLA initialization work the way it was supposed to.  A patch to handle it properly exists (bug 69517) and I plan (hope) to dust it off for GCC 8 and submit it.
Comment 7 Jakub Jelinek 2017-03-31 13:14:51 UTC
*** Bug 80269 has been marked as a duplicate of this bug. ***
Comment 8 Jakub Jelinek 2017-03-31 14:51:22 UTC
So far I have:
--- gcc/cp/typeck2.c.jj	2017-03-02 08:08:42.000000000 +0100
+++ gcc/cp/typeck2.c	2017-03-31 15:36:54.366928789 +0200
@@ -739,7 +739,9 @@ split_nonconstant_init (tree dest, tree
 
   if (TREE_CODE (init) == TARGET_EXPR)
     init = TARGET_EXPR_INITIAL (init);
-  if (TREE_CODE (init) == CONSTRUCTOR)
+  if (TREE_CODE (init) == CONSTRUCTOR
+      || (TREE_CODE (init) == STRING_CST
+	  && array_of_runtime_bound_p (TREE_TYPE (dest))))
     {
       init = cp_fully_fold (init);
       code = push_stmt_list ();
@@ -1066,7 +1068,7 @@ digest_init_r (tree type, tree init, boo
 		}
 	    }
 
-	  if (type != TREE_TYPE (init))
+	  if (type != TREE_TYPE (init) && !array_of_runtime_bound_p (type))
 	    {
 	      init = copy_node (init);
 	      TREE_TYPE (init) = type;
--- gcc/cp/init.c.jj	2017-03-21 08:04:13.000000000 +0100
+++ gcc/cp/init.c	2017-03-31 16:38:18.346535659 +0200
@@ -4199,7 +4199,12 @@ build_vec_init (tree base, tree maxindex
   else if (from_array)
     {
       if (init)
-	/* OK, we set base2 above.  */;
+	{
+	  /* OK, we set base2 above.  */
+	  if (TREE_CODE (init) == STRING_CST
+	      && array_of_runtime_bound_p (atype))
+	    empty_list = true;
+	}
       else if (CLASS_TYPE_P (type)
 	       && ! TYPE_HAS_DEFAULT_CONSTRUCTOR (type))
 	{
and the remaining part is changing build_vec_init, so that it will perform the memcpy (MEM_REF = STRING_CST) followed by the initialization of the rest.
Comment 9 Jakub Jelinek 2017-04-01 13:54:31 UTC
Jason said he has a patch for this, so reassigning.
Comment 10 Jason Merrill 2017-04-03 21:16:12 UTC
Author: jason
Date: Mon Apr  3 21:15:36 2017
New Revision: 246662

URL: https://gcc.gnu.org/viewcvs?rev=246662&root=gcc&view=rev
Log:
	PR sanitizer/79993 - ICE with VLA initialization from string

	PR c++/69487 - wrong VLA initialization from string
	* init.c (finish_length_check): Split out from build_vec_init.
	(build_vec_init): Handle STRING_CST.
	* typeck2.c (split_nonconstant_init): Handle STRING_CST.
	(digest_init_r): Don't give a STRING_CST VLA type.

Added:
    trunk/gcc/testsuite/g++.dg/asan/pr78201.C
    trunk/gcc/testsuite/g++.dg/ext/vla17.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/init.c
    trunk/gcc/cp/typeck2.c
Comment 11 Jakub Jelinek 2017-04-04 05:20:36 UTC
Fixed on the trunk.
Comment 12 Jakub Jelinek 2017-10-10 13:25:15 UTC
GCC 5 branch is being closed
Comment 13 Jakub Jelinek 2018-10-26 11:45:31 UTC
GCC 6 branch is being closed, fixed in 7.x.