Small Device C Compiler suite

On Thu, 25 Oct 2012, Maarten Brock wrote:

> Hi all,
>
> In recent regression tests there are a few warnings about pointers losing
> their const qualifier in code like this:
> char *s = "abc";
>
> Now my question is if this "abc" string literal is constant or not?
> SDCC thinks it is because it likes to put it in unmodifiable code memory
> (ROM).
> But GCC thinks it's not. It even allows *s='A'; So apparently it didn't
> even bother to put it in a write-protected segment for the MMU.
>
> Can anyone point to the definitive answer?
>
> Maarten

Section 6.4.5 "String literals" of ISO/IEC 9899:1999 among other things 
says:

   In translation phase 7, a byte or code of value zero is appended to
   each multibyte character sequence that results from a string literal or
   literals. The multibyte character sequence is then used to initialize an
   array of static storage duration and length just sufficient to contain
   the sequence. For character string literals, the array elements have
   type char, and are initialized with the individual bytes of the
   multibyte character sequence; [similarly for wide string literals]

So my reading of this is that the individual characters are not const.
However, the next paragraph in the standard states:

   If the program attempts to modify such an array, the behavior is
   undefined.

So even if it's not const, we are still allowed to place the array in the 
(presumably read-only) code address space.

   Erik

On Thu, 25 Oct 2012, Maarten Brock wrote:

> Hi all,
>
> In recent regression tests there are a few warnings about pointers losing
> their const qualifier in code like this:
> char *s = "abc";
>
> Now my question is if this "abc" string literal is constant or not?
> SDCC thinks it is because it likes to put it in unmodifiable code memory
> (ROM).
> But GCC thinks it's not. It even allows *s='A'; So apparently it didn't
> even bother to put it in a write-protected segment for the MMU.
>
> Can anyone point to the definitive answer?
>
> Maarten

Not definitive in a standards conformance sense, but the C Rationale 
document that parallels the actual standard is often helpful to read 
between the lines of the standard itself. In this case it says:

   String literals are not required to be modifiable. This specification
   allows implementations to share copies of strings with identical text,
   to place string literals in read-only memory, and to perform certain
   optimizations. However, string literals do not have the type array of
   const char in order to avoid the problems of pointer type checking,
   particularly with library functions, since assigning a pointer to
   const char to a plain pointer to char is not valid. Those members of
   the C89 Committee who insisted that string literals should be
   modifiable were content to have this practice designated a common
   extension (see paragraph J.5.5).

This seems to agree with my earlier interpretation.

   Erik

Thank you Erik,

>   String literals are not required to be modifiable. This specification
>   allows implementations to share copies of strings with identical text,
>   to place string literals in read-only memory, and to perform certain
>   optimizations. However, string literals do not have the type array of
>   const char in order to avoid the problems of pointer type checking,
>   particularly with library functions, since assigning a pointer to
>   const char to a plain pointer to char is not valid.

>   If the program attempts to modify such an array, the behavior is
>   undefined.

So it is ok that SDCC places the string literal in ROM, but it is wrong to
make it const implicitly. And the behavior of modifying the contents at
runtime should not be checked (or even executed) in the regression tests.

The library remark seems to be covering legacy code where functions expect
a (non-const) char* parameter but never really modify the contents. And to
allow the user to pass it a string literal.

Thanks again,
Maarten

In gcc you can do the following:

char *p = "ABC";
p[0] = 'a';

The compiler will not complain but the program will crash with 
Segmentation fault (at least gcc 4.6.3 on my amd64 machine). gcc puts 
literal strings in ro section ( .section .rodata gas directive). 
Interesting that

"ABC"[0] = 'a';

throws the "warning: assignment of read-only location ‘"AAA"[0]’ 
[enabled by default]" warning.

You can use -Wwrite-strings command line option to turn "warning: 
initialization discards ‘const’ qualifier from pointer target type 
[enabled by default]" warnings on or use -fno-const-strings to put 
strings in normal (no ro) section.


I don't know what is written in the standard but IMHO it is better that 
the compiler complains when the constant literal string is assigned to 
non-constant pointer since it (potentially) causes the run time error 
(which is never shown in sdcc case).

Putting literals strings into rw memory by default doesn't make sense in 
embedded systems since it is an overhead in RAM, ROM and instruction 
cycles consumption.

An option would be to implement -fno-const-strings in sdcc too, so the 
user can choose where the string literals are allocated...

And now the answer to your question from my point of view: in sdcc 
string literal is constants an the compiler should throw an error when 
assigning it to a non constant char pointer.

Borut

On 25. 10. 2012 01:06, Maarten Brock wrote:
> Hi all,
>
> In recent regression tests there are a few warnings about pointers losing
> their const qualifier in code like this:
> char *s = "abc";
>
> Now my question is if this "abc" string literal is constant or not?
> SDCC thinks it is because it likes to put it in unmodifiable code memory
> (ROM).
> But GCC thinks it's not. It even allows *s='A'; So apparently it didn't
> even bother to put it in a write-protected segment for the MMU.
>
> Can anyone point to the definitive answer?
>
> Maarten
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_sfd2d_oct
> _______________________________________________
> sdcc-devel mailing list
> sdcc-devel@...
> https://lists.sourceforge.net/lists/listinfo/sdcc-devel
>

While this is indeed the best default behaviour, ideally, the user should
be able to prescribe where to place string literals (and for that matter,
compound literals too). That would then result in various qualifiers
attached to the resulting pointer.

While there is established practice in most mcu-targeted compilers to have
a mechanism to place variables into the various memory spaces (memory
classes, named address spaces); whatever that mechanism is, it never
applies to string/compound literals, which is IMHO a pity (read: a lost
opportunity for the user to grab full control over his code and data). And
there is no hint from the standard and associated documents, in which
direction to go. The avr-gcc project struggles with this, too.

I wrote few thoughts about this already in
http://sourceforge.net/mailarchive/message.php?msg_id=28459968

JW



----- Original Message ---------------

>Thank you Erik,
>
>>   String literals are not required to be modifiable. This specification
>>   allows implementations to share copies of strings with identical text,
>>   to place string literals in read-only memory, and to perform certain
>>   optimizations. However, string literals do not have the type array of
>>   const char in order to avoid the problems of pointer type checking,
>>   particularly with library functions, since assigning a pointer to
>>   const char to a plain pointer to char is not valid.
>
>>   If the program attempts to modify such an array, the behavior is
>>   undefined.
>
>So it is ok that SDCC places the string literal in ROM, but it is wrong to
>make it const implicitly. And the behavior of modifying the contents at
>runtime should not be checked (or even executed) in the regression tests.
>
>The library remark seems to be covering legacy code where functions expect
>a (non-const) char* parameter but never really modify the contents. And to
>allow the user to pass it a string literal.
>
>Thanks again,
>Maarten
>

On 25.10.2012 08:41, Borut Ražem wrote:

> And now the answer to your question from my point of view: in sdcc
> string literal is constants an the compiler should throw an error when
> assigning it to a non constant char pointer.

No. The standard allows assigning it to a non-constant pointer, so we
have to allow it, too. And it probably would break quite some legacy
code to throw an error.
Emitting a warning is the right thing to do.

Philipp

> In gcc you can do the following:
>
> char *p = "ABC";
> p[0] = 'a';
>
> The compiler will not complain but the program will crash with
> Segmentation fault (at least gcc 4.6.3 on my amd64 machine). gcc puts
> literal strings in ro section ( .section .rodata gas directive).
> Interesting that
>
> "ABC"[0] = 'a';
>
> throws the "warning: assignment of read-only location ‘"AAA"[0]’
> [enabled by default]" warning.

Ah, so it segmentation faults if a regression test fails. That's also a
way to fail. (gcc-torture-execute-20051104-1.c)

> You can use -Wwrite-strings command line option to turn "warning:
> initialization discards ‘const’ qualifier from pointer target type
> [enabled by default]" warnings on or use -fno-const-strings to put
> strings in normal (no ro) section.

And which of these options are used in our host regression tests? Because
I don't see the warning.

> I don't know what is written in the standard but IMHO it is better that
> the compiler complains when the constant literal string is assigned to
> non-constant pointer since it (potentially) causes the run time error
> (which is never shown in sdcc case).
>
> Putting literals strings into rw memory by default doesn't make sense in
> embedded systems since it is an overhead in RAM, ROM and instruction
> cycles consumption.
>
> An option would be to implement -fno-const-strings in sdcc too, so the
> user can choose where the string literals are allocated...
>
> And now the answer to your question from my point of view: in sdcc
> string literal is constants an the compiler should throw an error when
> assigning it to a non constant char pointer.
>
> Borut

So it seems we agree to disagree with the standard! String literals should
be constant. And warnings should be given. But then I propose to change
the gcc tests to conform and not use char *s = "123" in there.

Maarten

>
> On 25. 10. 2012 01:06, Maarten Brock wrote:
>> Hi all,
>>
>> In recent regression tests there are a few warnings about pointers
>> losing
>> their const qualifier in code like this:
>> char *s = "abc";
>>
>> Now my question is if this "abc" string literal is constant or not?
>> SDCC thinks it is because it likes to put it in unmodifiable code memory
>> (ROM).
>> But GCC thinks it's not. It even allows *s='A'; So apparently it didn't
>> even bother to put it in a write-protected segment for the MMU.
>>
>> Can anyone point to the definitive answer?
>>
>> Maarten

Am 25.10.2012 09:28, schrieb Maarten Brock:
>> In gcc you can do the following:
>>
>> char *p = "ABC";
>> p[0] = 'a';
>>
>> The compiler will not complain but the program will crash with
>> Segmentation fault (at least gcc 4.6.3 on my amd64 machine). gcc puts
>> literal strings in ro section ( .section .rodata gas directive).
>> Interesting that
>>
>> "ABC"[0] = 'a';
>>
>> throws the "warning: assignment of read-only location ‘"AAA"[0]’
>> [enabled by default]" warning.
> 
> Ah, so it segmentation faults if a regression test fails. That's also a
> way to fail. (gcc-torture-execute-20051104-1.c)
> 
>> You can use -Wwrite-strings command line option to turn "warning:
>> initialization discards ‘const’ qualifier from pointer target type
>> [enabled by default]" warnings on or use -fno-const-strings to put
>> strings in normal (no ro) section.
> 
> And which of these options are used in our host regression tests? Because
> I don't see the warning.
> 
>> I don't know what is written in the standard but IMHO it is better that
>> the compiler complains when the constant literal string is assigned to
>> non-constant pointer since it (potentially) causes the run time error
>> (which is never shown in sdcc case).
>>
>> Putting literals strings into rw memory by default doesn't make sense in
>> embedded systems since it is an overhead in RAM, ROM and instruction
>> cycles consumption.
>>
>> An option would be to implement -fno-const-strings in sdcc too, so the
>> user can choose where the string literals are allocated...
>>
>> And now the answer to your question from my point of view: in sdcc
>> string literal is constants an the compiler should throw an error when
>> assigning it to a non constant char pointer.
>>
>> Borut
> 
> So it seems we agree to disagree with the standard! String literals should
> be constant. And warnings should be given. But then I propose to change
> the gcc tests to conform and not use char *s = "123" in there.
> 
> Maarten

To me, regression test gcc-torture-execute-20051104-1.c is somewhat like
this:

if(0)
x /= 0;

Now, dividing by zero would be undefined behaviour. But a conforming
compiler has to accept this code. Just as it should accept

char *a = "bla";
if(0)
a[0] = 0;

If instead of the plain 0 in the condition there is something that the
compiler cannot decide at compile time it still has to behave correctly.
Undefine dbehaviour may only happen when the code in question would
actually be executed.

IMO the test gcc-torture-execute-20051104-1.c should stay as it is, and
we disable the sdcc warning using a #pragma (note that the standard
AFAIK never forbids emitting a warning for conforming code).

Philipp

Philipp,

>> Ah, so it segmentation faults if a regression test fails. That's also a
>> way to fail. (gcc-torture-execute-20051104-1.c)

> To me, regression test gcc-torture-execute-20051104-1.c is somewhat like
> this:
>
> if(0)
> x /= 0;
>
> Now, dividing by zero would be undefined behaviour. But a conforming
> compiler has to accept this code. Just as it should accept
>
> char *a = "bla";
> if(0)
> a[0] = 0;
>
> If instead of the plain 0 in the condition there is something that the
> compiler cannot decide at compile time it still has to behave correctly.
> Undefined behaviour may only happen when the code in question would
> actually be executed.
>
> IMO the test gcc-torture-execute-20051104-1.c should stay as it is, and
> we disable the sdcc warning using a #pragma (note that the standard
> AFAIK never forbids emitting a warning for conforming code).
>
> Philipp

I agree with your assertions, but disagree with your conclusion. This test
has two issues. The first is that SDCC gives a warning for the assignment
of the string literal and GCC stays quiet for some unknown reason. If we
want to test that SDCC accepts this assignment, the expected warning
should be disabled (suppressed) with the #pragma. That is exactly why I
introduced this #pragma a long time ago. But I would prefer a specific
regression test to accept char *s = "abc" and never use this construct in
any other test as an accidental side effect test.

This goes for all regression tests that generate expected warnings, so
please do not commit tests that generate warnings. If the warning
generating code is not the core of the issue to be tested change it to
conforming code. IMHO only tests that generate warnings that still need to
be fixed can stay as a reminder (e.g. long-long or double support).

The second issue is that when for some reason the regression test fails it
will execute code with undefined behavior. I find that a very unfriendly
way to fail. On a machine with MMU like the host with GCC it can generate
a segmentation fault. On an embedded MCU it most likely performs a NOP,
but we might as well lock the MCU up (e.g. in _gptrput.c).

to Borut,

>>> I don't know what is written in the standard but IMHO it is better that
>>> the compiler complains when the constant literal string is assigned to
>>> non-constant pointer since it (potentially) causes the run time error
>>> (which is never shown in sdcc case).
>>>
>>> And now the answer to your question from my point of view: in sdcc
>>> string literal is constants an the compiler should throw an error when
>>> assigning it to a non constant char pointer.
>>
>> So it seems we agree to disagree with the standard! String literals
>> should be constant. And warnings should be given. But then I propose to
>> change the gcc tests to conform and not use char *s = "123" in there.
>>
>> Maarten

I'm with Philipp here that we should not generate an error but stick to a
warning.

>>> Putting literals strings into rw memory by default doesn't make sense
>>> in embedded systems since it is an overhead in RAM, ROM and instruction
>>> cycles consumption.
>>>
>>> Borut

I fully agree.

Maarten

Am 25.10.2012 11:33, schrieb Maarten Brock:
> Philipp,
> 
>>> Ah, so it segmentation faults if a regression test fails. That's also a
>>> way to fail. (gcc-torture-execute-20051104-1.c)
> 
>> To me, regression test gcc-torture-execute-20051104-1.c is somewhat like
>> this:
>>
>> if(0)
>> x /= 0;
>>
>> Now, dividing by zero would be undefined behaviour. But a conforming
>> compiler has to accept this code. Just as it should accept
>>
>> char *a = "bla";
>> if(0)
>> a[0] = 0;
>>
>> If instead of the plain 0 in the condition there is something that the
>> compiler cannot decide at compile time it still has to behave correctly.
>> Undefined behaviour may only happen when the code in question would
>> actually be executed.
>>
>> IMO the test gcc-torture-execute-20051104-1.c should stay as it is, and
>> we disable the sdcc warning using a #pragma (note that the standard
>> AFAIK never forbids emitting a warning for conforming code).
>>
>> Philipp
> 
> I agree with your assertions, but disagree with your conclusion. This test
> has two issues. The first is that SDCC gives a warning for the assignment
> of the string literal and GCC stays quiet for some unknown reason. If we
> want to test that SDCC accepts this assignment, the expected warning
> should be disabled (suppressed) with the #pragma. That is exactly why I
> introduced this #pragma a long time ago. But I would prefer a specific
> regression test to accept char *s = "abc" and never use this construct in
> any other test as an accidental side effect test.
> 
> This goes for all regression tests that generate expected warnings, so
> please do not commit tests that generate warnings. If the warning
> generating code is not the core of the issue to be tested change it to
> conforming code. IMHO only tests that generate warnings that still need to
> be fixed can stay as a reminder (e.g. long-long or double support).

But that way we do not test for regressions in interactions of
warning-generating constructs and other things.
I'd prefer to just disable the warning for these tests.
That is the way we handle it e.g. with tests that have a function with
an unused parameter. Or do you suggest to also rewrite these tests to
remove the unused parameters and introduce exactly one test where there
is an unused function parameter?
What do you mean by "conforming code"? Code that conforms to the
standard? Code that generates no warnings?

> 
> The second issue is that when for some reason the regression test fails it
> will execute code with undefined behavior. I find that a very unfriendly
> way to fail. On a machine with MMU like the host with GCC it can generate
> a segmentation fault. On an embedded MCU it most likely performs a NOP,
> but we might as well lock the MCU up (e.g. in _gptrput.c).

You seem to assume that failing this test always means that the
condition "s.name [s.len] != 0" will be true at runtime, and the test
author just used the write to the string as a substitute for a failed
assertion.
However, it could very well be that the write to string in the C code is
required to trigger the bug we want to test for. And we might very well
want to test that the compiler does not run into problems at compile
time when seeing such code (where "such" could mean "assignment to
string where we cannot decide at compile-time if it will be executed" or
something else that can be found in the testcase).
I would prefer to keep the tests from gcc as close to what they are in
gcc. Both to make it easier for people looking at them and their origin
later and to ensure that in our own tests, we try to cover a large
fraction of the issues covered by gcc's tests in gcc.

Philipp

On 25. 10. 2012 09:06, Philipp Klaus Krause wrote:
> On 25.10.2012 08:41, Borut Ražem wrote:
>
>> And now the answer to your question from my point of view: in sdcc
>> string literal is constants an the compiler should throw an error when
>> assigning it to a non constant char pointer.
> No. The standard allows assigning it to a non-constant pointer, so we
> have to allow it, too. And it probably would break quite some legacy
> code to throw an error.
> Emitting a warning is the right thing to do.
>
> Philipp

I had a warning in mind when I wrote "should throw an error", so I agree 
with Philipp.

Borut

Am 25.10.2012 11:33, schrieb Maarten Brock:

> This goes for all regression tests that generate expected warnings, so
> please do not commit tests that generate warnings.

When adding regression tests in the future, I will use #pragma to
disable sdcc warnings, unless they are warnings about unimplemented
features.

Philipp

I had a warning in mind when I wrote "should throw an error", so I agree
with Philipp.

Borut

On Thu, Oct 25, 2012 at 9:06 AM, Philipp Klaus Krause <pkk@...> wrote:

> On 25.10.2012 08:41, Borut Ražem wrote:
>
> > And now the answer to your question from my point of view: in sdcc
> > string literal is constants an the compiler should throw an error when
> > assigning it to a non constant char pointer.
>
> No. The standard allows assigning it to a non-constant pointer, so we
> have to allow it, too. And it probably would break quite some legacy
> code to throw an error.
> Emitting a warning is the right thing to do.
>
> Philipp
>
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_sfd2d_oct
> _______________________________________________
> sdcc-devel mailing list
> sdcc-devel@...
> https://lists.sourceforge.net/lists/listinfo/sdcc-devel
>

>> You can use -Wwrite-strings command line option to turn "warning:
>> initialization discards ‘const’ qualifier from pointer target type
>> [enabled by default]" warnings on or use -fno-const-strings to put
>> strings in normal (no ro) section.

> And which of these options are used in our host regression tests? Because
> I don't see the warning.

None.

Borut

On Thu, Oct 25, 2012 at 9:28 AM, Maarten Brock <sourceforge.brock@...:

> > In gcc you can do the following:
> >
> > char *p = "ABC";
> > p[0] = 'a';
> >
> > The compiler will not complain but the program will crash with
> > Segmentation fault (at least gcc 4.6.3 on my amd64 machine). gcc puts
> > literal strings in ro section ( .section .rodata gas directive).
> > Interesting that
> >
> > "ABC"[0] = 'a';
> >
> > throws the "warning: assignment of read-only location ‘"AAA"[0]’
> > [enabled by default]" warning.
>
> Ah, so it segmentation faults if a regression test fails. That's also a
> way to fail. (gcc-torture-execute-20051104-1.c)
>
> > You can use -Wwrite-strings command line option to turn "warning:
> > initialization discards ‘const’ qualifier from pointer target type
> > [enabled by default]" warnings on or use -fno-const-strings to put
> > strings in normal (no ro) section.
>
> And which of these options are used in our host regression tests? Because
> I don't see the warning.
>
> > I don't know what is written in the standard but IMHO it is better that
> > the compiler complains when the constant literal string is assigned to
> > non-constant pointer since it (potentially) causes the run time error
> > (which is never shown in sdcc case).
> >
> > Putting literals strings into rw memory by default doesn't make sense in
> > embedded systems since it is an overhead in RAM, ROM and instruction
> > cycles consumption.
> >
> > An option would be to implement -fno-const-strings in sdcc too, so the
> > user can choose where the string literals are allocated...
> >
> > And now the answer to your question from my point of view: in sdcc
> > string literal is constants an the compiler should throw an error when
> > assigning it to a non constant char pointer.
> >
> > Borut
>
> So it seems we agree to disagree with the standard! String literals should
> be constant. And warnings should be given. But then I propose to change
> the gcc tests to conform and not use char *s = "123" in there.
>
> Maarten
>
> >
> > On 25. 10. 2012 01:06, Maarten Brock wrote:
> >> Hi all,
> >>
> >> In recent regression tests there are a few warnings about pointers
> >> losing
> >> their const qualifier in code like this:
> >> char *s = "abc";
> >>
> >> Now my question is if this "abc" string literal is constant or not?
> >> SDCC thinks it is because it likes to put it in unmodifiable code memory
> >> (ROM).
> >> But GCC thinks it's not. It even allows *s='A'; So apparently it didn't
> >> even bother to put it in a write-protected segment for the MMU.
> >>
> >> Can anyone point to the definitive answer?
> >>
> >> Maarten
>
>
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_sfd2d_oct
> _______________________________________________
> sdcc-devel mailing list
> sdcc-devel@...
> https://lists.sourceforge.net/lists/listinfo/sdcc-devel
>

>>> You can use -Wwrite-strings command line option to turn "warning:
>>> initialization discards ‘const’ qualifier from pointer target type
>>> [enabled by default]" warnings on or use -fno-const-strings to put
>>> strings in normal (no ro) section.
>
>> And which of these options are used in our host regression tests?
>> Because I don't see the warning.
>
> None.

I don't get it. Will using -Wwrite-strings enable or disable the warning?
And is it enabled by default or not? And if so, what is then enabled? The
option or the warning? Because how I read the above statement is that the
option is enabled by default and it should be generating the warning.

Maarten

Am 25.10.2012 05:17, schrieb Erik Petrich:
> [...]
>     String literals are not required to be modifiable. This specification
>     allows implementations to share copies of strings with identical text,
>     to place string literals in read-only memory, and to perform certain
>     optimizations. However, string literals do not have the type array of
>     const char in order to avoid the problems of pointer type checking,
>     particularly with library functions, since assigning a pointer to
>     const char to a plain pointer to char is not valid. Those members of
>     the C89 Committee who insisted that string literals should be
>     modifiable were content to have this practice designated a common
>     extension (see paragraph J.5.5).
>

Well, early this year I had an interesting experience. Some kids were 
using avr-gcc to program Arduinos, and I helped them. So the following 
is based on GCC with unknown options set.

They had 2 or 3 identical literal strings, each assigned to different 
character pointers (char *p, no const). When modifying one of the 
strings, the "other" strings were modified, too. So the compiler decided 
to put all these identical string literals into one shared place, which 
was not read-only. And all the pointers stored the same address.

The solution in this case was to use character arrays instead of pointers.

Just my €0.02

Bodo

> Am 25.10.2012 11:33, schrieb Maarten Brock:
>> Philipp,
>>
>>>> Ah, so it segmentation faults if a regression test fails. That's also
>>>> a way to fail. (gcc-torture-execute-20051104-1.c)
>>
>>> To me, regression test gcc-torture-execute-20051104-1.c is somewhat
>>> like this:
>>>
>>> if(0)
>>> x /= 0;
>>>
>>> Now, dividing by zero would be undefined behaviour. But a conforming
>>> compiler has to accept this code. Just as it should accept
>>>
>>> char *a = "bla";
>>> if(0)
>>> a[0] = 0;
>>>
>>> If instead of the plain 0 in the condition there is something that the
>>> compiler cannot decide at compile time it still has to behave
>>> correctly.
>>> Undefined behaviour may only happen when the code in question would
>>> actually be executed.
>>>
>>> IMO the test gcc-torture-execute-20051104-1.c should stay as it is, and
>>> we disable the sdcc warning using a #pragma (note that the standard
>>> AFAIK never forbids emitting a warning for conforming code).
>>>
>>> Philipp
>>
>> I agree with your assertions, but disagree with your conclusion. This
>> test has two issues. The first is that SDCC gives a warning for the
>> assignment
>> of the string literal and GCC stays quiet for some unknown reason. If we
>> want to test that SDCC accepts this assignment, the expected warning
>> should be disabled (suppressed) with the #pragma. That is exactly why I
>> introduced this #pragma a long time ago. But I would prefer a specific
>> regression test to accept char *s = "abc" and never use this construct
>> in any other test as an accidental side effect test.
>>
>> This goes for all regression tests that generate expected warnings, so
>> please do not commit tests that generate warnings. If the warning
>> generating code is not the core of the issue to be tested change it to
>> conforming code. IMHO only tests that generate warnings that still need
>> to be fixed can stay as a reminder (e.g. long-long or double support).
>
> But that way we do not test for regressions in interactions of
> warning-generating constructs and other things.

I opened up the PR of this particular bug and the assignment of the
literal is not part of the reported problem. It's just the lazy
implementation of the GCC developer. So replacing the empty string literal
"" with an array containing an empty string fixes both problems.

> I'd prefer to just disable the warning for these tests.
> That is the way we handle it e.g. with tests that have a function with
> an unused parameter. Or do you suggest to also rewrite these tests to
> remove the unused parameters and introduce exactly one test where there
> is an unused function parameter?

Unused parameters can also be marked used by just dummy reading them:
foo(char c)
{
  c;    // dummy read
}

And yes, when the unused parameter has no purpose it is better removed.
Suppressing warnings should be a last resort but is still better than
keeping the warning.

> What do you mean by "conforming code"? Code that conforms to the
> standard? Code that generates no warnings?

Code that generates no warnings.

>> The second issue is that when for some reason the regression test fails
>> it
>> will execute code with undefined behavior. I find that a very unfriendly
>> way to fail. On a machine with MMU like the host with GCC it can
>> generate
>> a segmentation fault. On an embedded MCU it most likely performs a NOP,
>> but we might as well lock the MCU up (e.g. in _gptrput.c).
>
> You seem to assume that failing this test always means that the
> condition "s.name [s.len] != 0" will be true at runtime, and the test
> author just used the write to the string as a substitute for a failed
> assertion.

No, I assume that the author didn't think things through. And you seem
unwilling to question/modify the code. I don't think the GCC regression
test are absolute.

> However, it could very well be that the write to string in the C code is
> required to trigger the bug we want to test for. And we might very well
> want to test that the compiler does not run into problems at compile
> time when seeing such code (where "such" could mean "assignment to
> string where we cannot decide at compile-time if it will be executed" or
> something else that can be found in the testcase).
> I would prefer to keep the tests from gcc as close to what they are in
> gcc. Both to make it easier for people looking at them and their origin
> later and to ensure that in our own tests, we try to cover a large
> fraction of the issues covered by gcc's tests in gcc.
>
> Philipp

Maarten

> Am 25.10.2012 05:17, schrieb Erik Petrich:
>> [...]
>>     String literals are not required to be modifiable. This
>> specification
>>     allows implementations to share copies of strings with identical
>> text,
>>     to place string literals in read-only memory, and to perform certain
>>     optimizations. However, string literals do not have the type array
>> of
>>     const char in order to avoid the problems of pointer type checking,
>>     particularly with library functions, since assigning a pointer to
>>     const char to a plain pointer to char is not valid. Those members of
>>     the C89 Committee who insisted that string literals should be
>>     modifiable were content to have this practice designated a common
>>     extension (see paragraph J.5.5).
>>
>
> Well, early this year I had an interesting experience. Some kids were
> using avr-gcc to program Arduinos, and I helped them. So the following
> is based on GCC with unknown options set.
>
> They had 2 or 3 identical literal strings, each assigned to different
> character pointers (char *p, no const). When modifying one of the
> strings, the "other" strings were modified, too. So the compiler decided
> to put all these identical string literals into one shared place, which
> was not read-only. And all the pointers stored the same address.
>
> The solution in this case was to use character arrays instead of pointers.
>
> Just my €0.02
>
> Bodo

This is one of many implementations of Undefined Behavior when writing
into string literals.

Maarten

On 25.10.2012 08:22, Maarten Brock wrote:
> Thank you Erik,
> 
>>   String literals are not required to be modifiable. This specification
>>   allows implementations to share copies of strings with identical text,
>>   to place string literals in read-only memory, and to perform certain
>>   optimizations. However, string literals do not have the type array of
>>   const char in order to avoid the problems of pointer type checking,
>>   particularly with library functions, since assigning a pointer to
>>   const char to a plain pointer to char is not valid.
> 
>>   If the program attempts to modify such an array, the behavior is
>>   undefined.
> 
> So it is ok that SDCC places the string literal in ROM, but it is wrong to
> make it const implicitly. And the behavior of modifying the contents at
> runtime should not be checked (or even executed) in the regression tests.

Hmm, if we make the simulators report an error on write to ROM (as we
now do for invalid instructions) we can handle this. And we catch many
cases of the much more common problem of writes through invalid
pointers, too!

Philipp

On Thu, Oct 25, 2012 at 9:10 PM, Maarten Brock <sourceforge.brock@...:

> >>> You can use -Wwrite-strings command line option to turn "warning:
> >>> initialization discards ‘const’ qualifier from pointer target type
> >>> [enabled by default]" warnings on or use -fno-const-strings to put
> >>> strings in normal (no ro) section.
> >
> >> And which of these options are used in our host regression tests?
> >> Because I don't see the warning.
> >
> > None.
>
> I don't get it. Will using -Wwrite-strings enable or disable the warning?
>

Using -Wwrite-strings will enable the warning.


> And is it enabled by default or not?


No, it is not.


> And if so, what is then enabled? The
> option or the warning? Because how I read the above statement is that the
> option is enabled by default and it should be generating the warning.


The "[enabled by default]" is part of warning message (I just copy&pasted
the complete warning message). I also don't understand what it means since
the warning is NOT enabled by default but explicitly by -Wwrite-strings
option.

Borut

Hi again,

I must apologize as it seems I misread the PR somehow.

>>>> IMO the test gcc-torture-execute-20051104-1.c should stay as it is,
>>>> and we disable the sdcc warning using a #pragma (note that the standard
>>>> AFAIK never forbids emitting a warning for conforming code).
>>>>
>>>> Philipp
>
> I opened up the PR of this particular bug and the assignment of the
> literal is not part of the reported problem. It's just the lazy
> implementation of the GCC developer. So replacing the empty string literal
> "" with an array containing an empty string fixes both problems.

I'm not sure what I read yesterday, but looking again today at
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23567
indicates that the string being read-only is exactly what is the problem
here.

>>> The second issue is that when for some reason the regression test fails
>>> it will execute code with undefined behavior. I find that a very
unfriendly
>>> way to fail. On a machine with MMU like the host with GCC it can generate
>>> a segmentation fault. On an embedded MCU it most likely performs a NOP,
>>> but we might as well lock the MCU up (e.g. in _gptrput.c).
>>
>> You seem to assume that failing this test always means that the
>> condition "s.name [s.len] != 0" will be true at runtime, and the test
>> author just used the write to the string as a substitute for a failed
>> assertion.
>
> No, I assume that the author didn't think things through.

I must take this back.

>> However, it could very well be that the write to string in the C code is
>> required to trigger the bug we want to test for.

It seems so. How I understand it now is that
  char *s = "";
  if (*s != 0)
    *s = 0;
was transformed by GCC into
  *s = (*s != 0) ? 0 : *s;

And thus it probably ALWAYS writes to the literal which can be in
read-only memory.

Maybe another optimization takes place after this transformation, like:
  *s = 0;

But that still writes to the literal.

So now my conclusion is that I agree with you that this test should stay
as it is and that the current regression test suite cannot detect this
problem. But the warning should still be suppressed though.

>>>   String literals are not required to be modifiable. This specification
>>>   allows implementations to share copies of strings with identical text,
>>>   to place string literals in read-only memory, and to perform certain
>>>   optimizations. However, string literals do not have the type array of
>>>   const char in order to avoid the problems of pointer type checking,
>>>   particularly with library functions, since assigning a pointer to
>>>   const char to a plain pointer to char is not valid.
>>
>>>   If the program attempts to modify such an array, the behavior is
>>>   undefined.
>>
>> So it is ok that SDCC places the string literal in ROM, but it is wrong to
>> make it const implicitly. And the behavior of modifying the contents at
>> runtime should not be checked (or even executed) in the regression tests.

Or we should check the SDCC implementation only.

> Hmm, if we make the simulators report an error on write to ROM (as we
> now do for invalid instructions) we can handle this. And we catch many
> cases of the much more common problem of writes through invalid
> pointers, too!
>
> Philipp

Well, to capture this bug we should. As mentioned for mcs51/ds390 this
could be done in _gptrput.c by locking up the MCU. For non-generic
pointers it will be caught at compile time since there is no instruction
to write to code memory in the mcs51. For other (von Neumann)
architectures it probably requires telling the simulator which memory is
ROM and which is RAM and adding the test to the simulator.

Maarten

Maarten