On 07/21/18 01:58, Martin Sebor wrote:
> On 07/20/2018 03:11 PM, Bernd Edlinger wrote:
>> I think I have found a valid test case where the latest patch
>> does generate invalid code:
>
> This is due to another bug in string_constant() that's latent
> on trunk but that's exposed by the patch. Trunk only "works"
> because of a bug/limitation in c_strlen() that the patch fixes
> or removes.
>
I am not sure if that falls under the definition of "latent" bug.
A latent bug would be something unexpected in a completely different
area of the compiler that is triggered by an improved optimization
or a fixed bug elsewhere.
> The underlying root cause is the handling of POINTER_PLUS
> expressions in string_constant(). The original code (before
> the handling of aggregates was added) just dealt with string
> constants. The new code does much more but doesn't get it
> quite right in these cases.
>
I think you should be much more careful with the expressions
you evaluate in string_constant. For instance when you look
at get_addr_base_and_unit_offset, you'll see it walks through
all kinds of type casts, VIEW_CONVERT_EXPR and MEM_REF with
nonzero displacement. When you see something like that do
not fold that on the assumption that the source code does not
have undefined behavior.
So I'd say you should add a check that there is no type
cast in the expression you parse. If that is the case,
your optimization will certainly be wrong.
As Richard already repeatedly said, and I can only emphasize
this once more:
The middle end does not follow the "C" standard too closely,
and it is also the C++, fortran, Ada and Go middle-end.
Even if the source code does not exhibit undefined behavior
the folding in the middle-end can introduce it.
Bernd.
> It shouldn't be too difficult to fix, but as valuable as
> the testing you are doing is, I'd really prefer to focus
> on one problem at a time and make incremental progress.
> It will make it easier to track down and fix regressions
> than lumping multiple fixes into a larger patch.
>
> Martin
>
>>
>> $ cat part.c
>> static const char a[3][8] = { "1234", "12345", "123456" };
>>
>> int main ()
>> {
>> volatile int i = 1;
>> int n = __builtin_strlen (*(&a[1]+i));
>>
>> if (n != 6)
>> __builtin_abort ();
>> }
>> $ gcc part.c -fdump-tree-original
>> $ ./a.out
>> Aborted (core dumped)
>> $ cat part.c.004t.original
>>
>> ;; Function main (null)
>> ;; enabled by -tree-original
>>
>>
>> {
>> volatile int i = 1;
>> int n = (ssizetype) (SAVE_EXPR <(long unsigned int) i * 8>) <= 5 ? (int)
>> (5 - (unsigned int) (SAVE_EXPR <(long unsigned int) i * 8>)) : 0;
>>
>> volatile int i = 1;
>> int n = (ssizetype) (SAVE_EXPR <(long unsigned int) i * 8>) <= 5 ?
>> (int) (5 - (unsigned int) (SAVE_EXPR <(long unsigned int) i * 8>)) : 0;
>> if (n != 6)
>> {
>> __builtin_abort ();
>> }
>> }
>> return 0;
>>
>>
>> string_constant is called with arg = (const char *) (&a[1] + (sizetype)
>> ((long unsigned int) i * 8))
>
