possibly undefined operation
luser- -droog
It's only modified once, right?
cat tos.c && make tos
#include <stdio.h>
int main(void) {
/* stack */
char stac[100];
char *tos = stac;
/* string */
char *str = "a string";
char *s;
/* push some elements */
for (s = str; *s; s++)
*tos++ = *s;
/* copy elements */
{
int n = tos-stac;
*tos++ = tos[-n]; /* undefined? */
}
*tos++ = 0;
puts(stac);
return 0;
}
/* eof */
cc -g -Wall tos.c -o tos
tos.c: In function 'main':
tos.c:19: warning: operation on 'tos' may be undefined
Ike Naar
> int n = tos-stac;
> *tos++ = tos[-n]; /* undefined? */ [line 19]
> }
> tos.c:19: warning: operation on 'tos' may be undefined
The behaviour is undefined (tos is inspected on the right hand side
of the assignment operator, and modified on the left hand side of the
assignment operator, with no intervening sequence point).
Did you indend to do this:
{
*tos++ = *stac;
}
Ike Naar
> Did you indend to do this:
I meant 'intend' ;-)
luser- -droog
> On 2010-12-30, luser- -droog <mijo...@yahoo.com> wrote:
>
> > How worried shoud I be about this warning?
> > {
> > int n = tos-stac;
> > *tos++ = tos[-n]; /* undefined? */ [line 19]
> > }
> > tos.c:19: warning: operation on 'tos' may be undefined
>
> The behaviour is undefined (tos is inspected on the right hand side
> of the assignment operator, and modified on the left hand side of the
> assignment operator, with no intervening sequence point).
>
>
> {
> *tos++ = *stac;
> }
I meant to do a little loop. Pushing the stack
with elements from the stack at a fixed offset
from the top.
/* copy elements */
{
int i,n;
i = n = tos-stac;
while (i--)
*tos++ = tos[-n]; /* undefined? */
}
It appears to execute fine. Even at -O3.
Ike Naar
> with elements from the stack at a fixed offset
> from the top.
>
> /* copy elements */
> {
> int i,n;
> i = n = tos-stac;
> while (i--)
> *tos++ = tos[-n]; /* undefined? */
> }
>
> It appears to execute fine. Even at -O3.
"Appears to execute fine" is one possible manifestation of undefined
behaviour.
christian.bau
> "Appears to execute fine" is one possible manifestation of undefined
> behaviour.
And "Appears to execute fine in development, but breaks when the
application is shipped to a customer" is another possible
manifestation :-)
luser- -droog
> How worried shoud I be about this warning?
> It's only modified once, right?
Rewriting with a pointer makes it all better.
> cat tos.c && make tos
> #include <stdio.h>
>
> int main(void) {
> /* stack */
> char stac[100];
> char *tos = stac;
>
> /* string */
> char *str = "a string";
> char *s;
>
> /* push some elements */
> for (s = str; *s; s++)
> *tos++ = *s;
>
> /* copy elements */
> {
> int i,n;
> i = n = tos-stac;
> while (i--)
> *tos++ = tos[-n]; /* undefined? */
> }
/* copy elements */
{
int n;
char *t;
n = tos-stac;
t = stac;
while (n--)
*tos++ = *t++;
Ike Naar
Yes.
> /* copy elements */
> {
> int n;
> char *t;
> n = tos-stac;
> t = stac;
> while (n--)
> *tos++ = *t++;
> }
Why not use memcpy from <string.h>, and replace the whole thing by
memcpy(tos, stac, tos-stac);
John Bode
wrote:
That's the *most common* manifestation in my experience.
luser- -droog
> On 2010-12-30, luser- -droog <mijo...@yahoo.com> wrote:
>
> > Rewriting with a pointer makes it all better.
>
> Yes.
>
> > /* copy elements */
> > {
> > int n;
> > char *t;
> > n = tos-stac;
> > t = stac;
> > while (n--)
> > *tos++ = *t++;
> > }
>
> Why not use memcpy from <string.h>, and replace the whole thing by
>
> memcpy(tos, stac, tos-stac);
Oh, I hadn't thought of that. *poof*
Seebs
You should fix the code.
> It's only modified once, right?
So what?
> *tos++ = tos[-n]; /* undefined? */
Yes, undefined. Even if it weren't "undefined", you would have no
information as to whether tos[-n] was indexing from the previous
or new value of tos. But it's undefined, so it's also allowed to
index into a half-loaded pointer which has garbage bits in its top
half, or something like that. In short, it's nonsense. Don't do that.
-s
--
Copyright 2010, all wrongs reversed. Peter Seebach / usenet...@seebs.net
http://www.seebs.net/log/ <-- lawsuits, religion, and funny pictures
http://en.wikipedia.org/wiki/Fair_Game_(Scientology) <-- get educated!
I am not speaking for my employer, although they do rent some of my opinions.
Gene
> On 2010-12-30, luser- -droog <mij...@yahoo.com> wrote:
> > How worried shoud I be about this warning?
>
> You should fix the code.
>
> > It's only modified once, right?
>
> So what?
>
> > *tos++ = tos[-n]; /* undefined? */
>
> Yes, undefined. Even if it weren't "undefined", you would have no
> information as to whether tos[-n] was indexing from the previous
> or new value of tos. But it's undefined, so it's also allowed to
> index into a half-loaded pointer which has garbage bits in its top
> half, or something like that. In short, it's nonsense. Don't do that.
>
So in case it isn't obvious to the OP, you need to introduce a sequence point. Easiest is probably to replace the single line with two:
char look_back = tos[-n];
*tos++ = look_back;
luser- -droog
> On Thursday, December 30, 2010 2:19:10 PM UTC-5, Seebs wrote:
> > On 2010-12-30, luser- -droog <mij...@yahoo.com> wrote:
> > > How worried shoud I be about this warning?
>
> > You should fix the code.
Conceded.
> > > It's only modified once, right?
>
> > So what?
mumble. mumble.
Oh, nothing.
> > > *tos++ = tos[-n]; /* undefined? */
>
> > Yes, undefined. Even if it weren't "undefined", you would have no
> > information as to whether tos[-n] was indexing from the previous
> > or new value of tos. But it's undefined, so it's also allowed to
> > index into a half-loaded pointer which has garbage bits in its top
> > half, or something like that. In short, it's nonsense. Don't do that.
Yes. The macro concealed the problem when I first examined it.
I can see now that I can't presume to guess where the increment
will occur.
> So in case it isn't obvious to the OP, you need to introduce a sequence point. Easiest is probably to replace the single line with two:
>
> char look_back = tos[-n];
> *tos++ = look_back;
Yes. But that won't propagate back to the 'actual' source problem.
I'd have to bust open the macro that works everywhere else.
extern Object os[OSSIZE], *tos;
#define push(o,op) ((tos-os<OSSIZE)? \
(*tos++ = o): \
(error(stackoverflow,OP_ ## op),null))
...
for (i=0; i<n; i++) { push(tos[-n],copy); }
I think I like the memcpy suggestion best. It seems the most
direct way to express the intent. It should be faster, too.
I can check for a full stack 'en masse' rather than checking
each move.
Gene
> > So in case it isn't obvious to the OP, you need to introduce a sequence point. Easiest is probably to replace the single line with two:
> >
> > char look_back = tos[-n];
> > *tos++ = look_back;
>
> Yes. But that won't propagate back to the 'actual' source problem.
> I'd have to bust open the macro that works everywhere else.
>
> extern Object os[OSSIZE], *tos;
> #define push(o,op) ((tos-os<OSSIZE)? \
> (*tos++ = o): \
> (error(stackoverflow,OP_ ## op),null))
>
>
> I think I like the memcpy suggestion best. It seems the most
> direct way to express the intent. It should be faster, too.
> I can check for a full stack 'en masse' rather than checking
> each move.
Okay. I did not grok that the problem was embedded in a macro. IMO macros aren't worth the trouble for this kind of thing. Use a function. If the call overhead is proven in profiling to be a bottleneck (almost never happens), arrange for the function to be inlined.
FWIW, I ran into a related problem working on a system with a copying garbage collector. A simple
frame->a[i] = cons(x, y);
would usually not work if the cons() caused a collection. In that case, the array frame->a would be copied to a new location and hence frame->a set to a new value. If the address of a[i] was calculated prior to the call to cons(), as this compiler liked to do, the assignment was to the old location of frame->a rather than the new. Havoc ensued. The only solution was to replace the above with
OBJECT *p = cons(x, y); frame->a[i] = p;
Ironically, I ended up wrapping this a macro ASSIGN(Dst, Src). Ouch.
luser- -droog
> Okay. I did not grok that the problem was embedded in a macro.
I hadn't mentioned it.
> IMO macros aren't worth the trouble for this kind of thing. Use a
> function. If the call overhead is proven in profiling to be a
> bottleneck (almost never happens), arrange for the function to be
> inlined.
I'll probably do this from now on. But I'm reluctant to replace the
macro at this time. It's still usable if I can remember not to
pass it an expression involving tos. And apparently gcc will
remind me if I forget.
I think the word 'may' in the warning suggested to me the possibility
of misdiagnosis.
Jorgen Grahn
I'm too lazy to read the whole original code, but remember that with
memcpy() the source and destination regions may not overlap.
/Jorgen
--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
Ike Naar
> On Thu, 2010-12-30, Ike Naar wrote:
>> memcpy(tos, stac, tos-stac);
>
> I'm too lazy to read the whole original code, but remember that with
> memcpy() the source and destination regions may not overlap.
If you're too lazy to read the original code, then it's sufficient
to look at the mempcy call, from which you can deduce that source
and destination are adjacent and do not overlap.
(the operation only makes sense if you assume that stac and tos
are valid pointers into the same array and stac<=tos; these
assumptions hold in the original code).
Jorgen Grahn
Well, I was too lazy to realize that too, but you're right of course.