This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Check the STRING_CSTs in varasm.c


Hi,


this is an updated version of my STRING_CSTs checking in varasm.c
patch.

It tries to answer the questions that were raised in th GO string literals
thread.

The answers are:
a) strings with TYPE_SIZE_UNIT == NULL do exist, but only for STRING_CSTs
in constructors of flexible array struct members.  Not for string literals.
b) In all cases where the STRING_CSTs do have a TYPE_SIZE_UNIT, the
DECL_SIZE_UNIT has the same value.
c) When STRING_CSTs do not have a TYPE_SIZE_UNIT, that is in the constructor
of a flexible array member.  In that case the TREE_STRING_LENGTH
determines the flexible array size.


It changes varasm's get_constant_size to return the TYPE_SIZE_UNIT of
a STRING_CST literal as it is, without increasing the storage size
to TREE_STRING_LENGTH.  I added an assertion to make sure that
all STRING_CSTs have a type size; size == 0 can happen for empty Ada
strings.

Furthermore it adds code to compare_constant to also compare the
STRING_CSTs TYPE_SIZE_UNIT if available.

So I want to remove that from get_constant_size in order to not change
the memory layout of GO and Ada strings, while still having them look
mostly like C string literals.

Furthermore I added one more consistency check to check_string_literal,
that makes sure that all STRING_CSTs that do have a TYPE_SIZE_UNIT,
the size matches the DECL_SIZE_UNIT.

Those newly discovered properties of string literals make the code in
c_strlen and friends a lot simpler.



Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.




On 08/17/18 15:53, Bernd Edlinger wrote:
> On 08/17/18 15:38, Richard Biener wrote:
>> On Fri, 17 Aug 2018, Bernd Edlinger wrote:
>>
>>> On 08/17/18 14:19, Richard Biener wrote:
>>>> On Fri, 17 Aug 2018, Bernd Edlinger wrote:
>>>>
>>>>> Richard Biener wrote:
>>>>>> +embedded @code{NUL} characters.  However, the
>>>>>> +@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
>>>>>> +is not part of the language string literal but appended by the front end.
>>>>>> +If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
>>>>>> +is one character shorter than @code{TREE_STRING_LENGTH}.
>>>>>> +Excess caracters other than one trailing @code{NUL} character are not
>>>>
>>>> characters btw.
>>>>
>>>
>>> thanks, updated.
>>>
>>>> I read the above that the string literal for
>>>>
>>>> char x[2] = "1";
>>>>
>>>> is actually "1\0\0" - there's one NUL that is not part of the language
>>>> string literal.  The second sentence then suggests that both \0
>>>> are removed because 2 is less than 3?
>>>>
>>>
>>> maybe 2 is a bad example, lets consider:
>>> char x[2000000000] = "1";
>>>
>>> That is a string_cst with STRING_LENGTH = 2, content = "2\0\0"
>>> the array_type is used on both x, and the string_cst,
>>> I was assuming that both tree objects refer to the same type object.
>>> which is char[0..2000000000-1]
>>
>> Oh, didn't realize we use char[200000000] for the STRING_CST.  Makes
>> my suggestion to use char[] instead not (very) much worse than the
>> existing practice then.
>>
>>> varasm assembles the bytes that are given by STRING_LENGTH
>>> and appends zeros as appropriate.
>>>
>>>> As said, having this extra semantics of a STRING_CST tied to
>>>> another tree node (its TREE_TYPE) looks ugly.
>>>>
>>>>>> +permitted.
>>>>>>
>>>>>> I find this very confusing and oppose to that change.  Can we get
>>>>>> back to the drawing board please?  If we want an easy way to
>>>>>> see whether a string is "properly" terminated then maybe we can
>>>>>> simply use a flag that gets set by build_string?
>>>>>>
>>>>>
>>>>> What I mean with that is the case like
>>>>> char x[2] = "123456";
>>>>>
>>>>> which is build_string(7, "123456"), but with a type char[2],
>>>>> so varasm throws away "3456\0".
>>>>
>>>> I think varasm throws away chars not because of the type of
>>>> the STRING_CST but because of the available storage in x.
>>>>
>>>
>>> But at other places we look at the type of the string_cst, don't we?
>>> Shouldn't those be the same?
>>
>> I think most (all?) places look at TREE_TYPE (TREE_TYPE (string))
>> only.  I'm not aware of users of the array domain of the array type
>> of a string - but I'm far from knowing GCC inside-out ;)
>>
> 
> Yes, I know, that happens to me as well on the first day after my holidays ;)
> 
>>>>> I want to say that this is not okay, the excess precision
>>>>> should only be used to strip the nul termination, in cases
>>>>> where it is intended to be a assembled as a not zero terminated
>>>>> string.  But maybe the wording could be improved?
>>>>
>>>> ISTR we always assemble a NUL in .strings to get string merging
>>>> working.
>>>>
>>>
>>> String merging is not working when the string is not explicitly
>>> NUL terminated, my followup patch here tries to fix that:
>>>
>>> [PATCH] Handle not explicitly zero terminated strings in merge sections
>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
>>
>> I'd have expected sth as simple as
>>
>>    if (merge_strings && str[thissize - 1] != '\0')
>>      thissize++;
>>
>> being appended in output_constant.
>>
> 
> Yes, but that can only be done in the .merge.str section,
> otherwise it would happen in structure initializers as well.
> And I would like to undo the case when Ada programs do
> 
> Process ("ABCD" & Ascii.NUL);
> 
> but not for embedded NUL in the string constant.
> like:
> 
> Process ("ABCD" & Acii.NUL & "EFGH");
> 
> 
> Bernd.
2018-08-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/generic.texi (STRING_CST): Update.
	* varasm.c (compare_constant): Compare type size of STRING_CSTs.
	(get_constant_size): Don't make STRING_CSTs larget than they are.
	(check_string_literal): New check function for STRING_CSTs.
	(output_constant): Use it.

diff -Npur gcc/doc/generic.texi gcc/doc/generic.texi
--- gcc/doc/generic.texi	2018-08-16 17:28:11.000000000 +0200
+++ gcc/doc/generic.texi	2018-08-22 10:17:29.852681466 +0200
@@ -1162,9 +1162,13 @@ These nodes represent string-constants.
 returns the length of the string, as an @code{int}.  The
 @code{TREE_STRING_POINTER} is a @code{char*} containing the string
 itself.  The string may not be @code{NUL}-terminated, and it may contain
-embedded @code{NUL} characters.  Therefore, the
-@code{TREE_STRING_LENGTH} includes the trailing @code{NUL} if it is
-present.
+embedded @code{NUL} characters.  However, the
+@code{TREE_STRING_LENGTH} always includes a trailing @code{NUL} that
+is not part of the language string literal but appended by the front end.
+If the string shall not be @code{NUL}-terminated the @code{TREE_TYPE}
+is one character shorter than @code{TREE_STRING_LENGTH}.
+Excess characters other than one trailing @code{NUL} character are not
+permitted.
 
 For wide string constants, the @code{TREE_STRING_LENGTH} is the number
 of bytes in the string, and the @code{TREE_STRING_POINTER}
@@ -1173,6 +1177,11 @@ target system (that is, as integers in t
 non-wide string constants are distinguished only by the @code{TREE_TYPE}
 of the @code{STRING_CST}.
 
+String constants may also be used for other purpose like assember
+constraints or attributes.  These have no @code{TREE_TYPE} and
+need no explicit @code{NUL}-termination.  Note there is always
+another @code{NUL}-byte beyond @code{TREE_STRING_LENGTH}.
+
 FIXME: The formats of string constants are not well-defined when the
 target system bytes are not the same width as host system bytes.
 
diff -Npur gcc/varasm.c gcc/varasm.c
--- gcc/varasm.c	2018-08-16 17:28:11.000000000 +0200
+++ gcc/varasm.c	2018-08-22 11:42:13.259002521 +0200
@@ -3146,7 +3146,9 @@ compare_constant (const tree t1, const t
       return FIXED_VALUES_IDENTICAL (TREE_FIXED_CST (t1), TREE_FIXED_CST (t2));
 
     case STRING_CST:
-      if (TYPE_MODE (TREE_TYPE (t1)) != TYPE_MODE (TREE_TYPE (t2)))
+      if (TYPE_MODE (TREE_TYPE (t1)) != TYPE_MODE (TREE_TYPE (t2))
+	  || int_size_in_bytes (TREE_TYPE (t1))
+	     != int_size_in_bytes (TREE_TYPE (t2)))
 	return 0;
 
       return (TREE_STRING_LENGTH (t1) == TREE_STRING_LENGTH (t2)
@@ -3303,8 +3305,7 @@ get_constant_size (tree exp)
   HOST_WIDE_INT size;
 
   size = int_size_in_bytes (TREE_TYPE (exp));
-  if (TREE_CODE (exp) == STRING_CST)
-    size = MAX (TREE_STRING_LENGTH (exp), size);
+  gcc_checking_assert (size >= 0);
   return size;
 }
 
@@ -4774,6 +4775,32 @@ initializer_constant_valid_for_bitfield_
   return false;
 }
 
+/* Check if a STRING_CST fits into the field.
+   Tolerate only the case when the NUL termination
+   does not fit into the field.   */
+
+static bool
+check_string_literal (tree string, unsigned HOST_WIDE_INT size)
+{
+  tree eltype = TREE_TYPE (TREE_TYPE (string));
+  unsigned HOST_WIDE_INT elts = tree_to_uhwi (TYPE_SIZE_UNIT (eltype));
+  const char *p = TREE_STRING_POINTER (string);
+  int len = TREE_STRING_LENGTH (string);
+  HOST_WIDE_INT mems = int_size_in_bytes (TREE_TYPE (string));
+
+  if (elts != 1 && elts != 2 && elts != 4)
+    return false;
+  if (len <= 0 || len % elts != 0)
+    return false;
+  if (size < (unsigned)len && size != len - elts)
+    return false;
+  if (memcmp (p + len - elts, "\0\0\0\0", elts) != 0)
+    return false;
+  if (mems >= 0 && mems != (HOST_WIDE_INT)size)
+    return false;
+  return true;
+}
+
 /* output_constructor outer state of relevance in recursive calls, typically
    for nested aggregate bitfields.  */
 
@@ -4942,6 +4969,7 @@ output_constant (tree exp, unsigned HOST
 	case STRING_CST:
 	  thissize
 	    = MIN ((unsigned HOST_WIDE_INT)TREE_STRING_LENGTH (exp), size);
+	  gcc_checking_assert (check_string_literal (exp, size));
 	  assemble_string (TREE_STRING_POINTER (exp), thissize);
 	  break;
 	case VECTOR_CST:

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]