[sdcc-devel] [ sdcc-Bugs-2472264 ] linking fails due to an assert added at #5292

[SourceForge.net <no...@so...>]

Bugs item #2472264, was opened at 2008-12-28 02:42
Message generated for change (Comment added) made by tecodev
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=2472264&group_id=599

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: linker
>Group: fixed
>Status: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: Steven Borley (sjborley)
>Assigned to: Nobody/Anonymous (nobody)
Summary: linking fails due to an assert added at #5292

Initial Comment:
I'm getting what appear to be getting a related problem to #2466950  (in sdcc rev #5303).

...
/Users/sdcc-builder/build/sdcc-build/orig/sdcc/as/link/mcs51/../lkaomf51.c:575: failed assertion `res == buff'

Looks like an assert was added after an an fgets() in rev #5292 and this is breaking the linking.

In sdcc/as/link/mcs51/../lkaomf51.c we have
...
	while(!feof(CDBin))
	{
		char *res;
		res = fgets(buff, sizeof(buff)-1, CDBin);
		assert(res == buff);

		if(!feof(CDBin)) switch(buff[0])
		{
		  ....
		}
	}
...

Looks to me that the assert triggers for a quite valid, natural exit of this loop when eof is reached.  I think the assert should be removed, or perhaps changed to...

		assert(ferror(CDBin) == 0);

which should pick up error conditions but not eof (not tested).



----------------------------------------------------------------------

>Comment By: Raphael Neider (tecodev)
Date: 2008-12-30 13:36

Message:
Reverted all potentially broken changes plus support/cpp/* to pre-5293,
committed as r5310.

This does not only remove all bogus asserts after fgets, but also some
'safe' asserts after fwrite/realpath/system---assert is simply not the
right approach, these call for more specific error handling (retry fwrite,
use non-canonicalized path, ignore failing subprocesses).

Hoping that this restores a working version,
Raphael


----------------------------------------------------------------------

Comment By: Steven Borley (sjborley)
Date: 2008-12-30 08:10

Message:
>>What I do not quite understand: I ran the regression tests on my box and
>>the compile farm runs them as well---successfully. Do they not use the
>>linker, possibly due to them consisting of only one source file each?
>>
>It seems that this happens only with -z "Produce SDCdb debug as
file[cdb]"
>command line option

The full list of flags I was passing to sdcc for linking were as follows
(it includes the CFLAGS)...

 $ sdcc -o <file> <files.rel> --debug --std-sdcc89 --main-return
--no-xinit-opt --opt-code-size --callee-saves <list>  -Wl-u --xram-size 0
--code-size 4096 --iram-size 256

It would be the --debug that's producing the -z flag to aslink.  If the
regression test are passing, does this mean that none check operation of
the --debug option?

Raphael,
Having found this problem I did go look at some of the other asserts and
could seen no problem with the others - no guarantee that I didn't miss
something but at least they've had a fresh pair of eyes.  I'd be happy with
a minimal revert.  We know where to look first if there is a next time :)

Steven

----------------------------------------------------------------------

Comment By: Raphael Neider (tecodev)
Date: 2008-12-30 00:31

Message:
OK, I can now at least reproduce the warnings on my current machine (Ubuntu
8.04): Somehow a -D_FORTIFY_SOURCE=x with x>0 seems to sneak into the gcc
command line on my Ubuntu 8.10 box, in turn enabling additional checks such
as the warn_unused_result one in the glibc headers (via features.h). Using
a gcc wrapper a la

#!/bin/sh
/usr/bin/gcc -D_FORTIFY_SOURCE=9 "$@";

in the PATH before /usr/bin (and a similar one for g++) should do the same
on other Linux distros as well.

Coming back to the broken source: Reverting the changes does not seem to
be as easy or useful as I expected in my previous posting. Some changes
(format string fixes, clarifying parens, clean.mk, src/pic/device.c) should
stay in (in my opinion), whereas the broken fgets() changes should be
reverted.
I am unsure about the fwrite()/realpath() changes: Iff the assert I
introduced fires, some output would have been lost or undefined memory
(unsuccessfully resolved paths) would be used. Just to make sure, I will
revert them as well.
Finally, hoping that the gcc guys will do something about the warnings
upstream, I shall also revert the changes in support/cpp/* to simplify your
upgrade task.

I still need to sort this out in the sources and will commit later today
(2008-12-30). If you or anyone else disagrees and rather favour(s) a "true"
revert, please say so.

Concerning possible offense: You did not offend me, your criticism is just
and appropriate.

Regards,
Raphael


----------------------------------------------------------------------

Comment By: Borut Ražem (borutr)
Date: 2008-12-29 22:43

Message:
> If they are just copies, there is no need to merge anything ;-) . And if
> they cause problems, should we not attempt to fix them?

They were just copies, but they are not any more, so they have to be
merged from now on :(
And they don't cause problems. If they would, they should be fixed in gcc
too, so the proper way wold be to report the bug to gcc team and then marge
(copy) the fix back to sdcc (at least in theory ;) )

> This partially depends on ones definition of "being broken".
> My "fix" was an attempt to catch more error conditions early rather
than
> silently ignore them, and an interrupted, short write resulting in some
> broken output file could be a rare but annoying compiler bug. I admit
that
> the asserts fire more often than I hoped and that they should be
removed,
> but on the long run, we should properly check the return values. Maybe
we
> need another set of wrappers Safe_fgets, Safe_fwrite, ...?

I understand your intent and agree with you. The problem is that even
things that seems trivial often need to be well considered. It happened
many times to me too and I'm sure it will happen again ;)

> What I do not quite understand: I ran the regression tests on my box
and
> the compile farm runs them as well---successfully. Do they not use the
> linker, possibly due to them consisting of only one source file each?

It seems that this happens only with -z "Produce SDCdb debug as file[cdb]"
command line option

> The warnings occur using unmodified sdcc Makefiles on my fully patched
> Ubuntu 8.10 with gcc 4.3.2 (I guess) and a recent glibc (>= 2.3
according
> to others having the same problems): the libc-headers included in that
> release add __attribute__((warn_unused_result)) to many functions,
whose
> result we choose to ignore.

gcc 4.3.2 is used for cross-compiling windows binaries on snapshot build
machine, but I haven't seen such warnings. I'll do some further
investigations...

Raphael, I hope I didn't offend you. It definitely wasn't my intent. If I
did, please accept my apologies.

Borut


----------------------------------------------------------------------

Comment By: Raphael Neider (tecodev)
Date: 2008-12-29 13:28

Message:
I agree with Borut as to revert the changes (I will try to get it done this
evening, but feel free to do it if you want to), but ...

> which are just copies of GCC's cpp, which make
> merging from GCC to SDCC mode difficult.

If they are just copies, there is no need to merge anything ;-) . And if
they cause problems, should we not attempt to fix them?

> "If It's Not Broken, Don't Fix It!"

This partially depends on ones definition of "being broken".
My "fix" was an attempt to catch more error conditions early rather than
silently ignore them, and an interrupted, short write resulting in some
broken output file could be a rare but annoying compiler bug. I admit that
the asserts fire more often than I hoped and that they should be removed,
but on the long run, we should properly check the return values. Maybe we
need another set of wrappers Safe_fgets, Safe_fwrite, ...?

What I do not quite understand: I ran the regression tests on my box and
the compile farm runs them as well---successfully. Do they not use the
linker, possibly due to them consisting of only one source file each?

> Which compiler (version) are you using?
> Any special command line options?

The warnings occur using unmodified sdcc Makefiles on my fully patched
Ubuntu 8.10 with gcc 4.3.2 (I guess) and a recent glibc (>= 2.3 according
to others having the same problems): the libc-headers included in that
release add __attribute__((warn_unused_result)) to many functions, whose
result we choose to ignore.
Casting to (void) does not help to remove the warning (as it seems by
intention of the gcc guys; assigning it to a variable does, but provokes an
"unused variable" warning unless the variable is used, e.g., as in the
untested '(void)temp = fwrite(...)').

Regretful regards,
Raphael


----------------------------------------------------------------------

Comment By: Borut Ražem (borutr)
Date: 2008-12-28 11:26

Message:
Raphael, I propose to revert all changes you made 2008-12-20 for "quieten
most compiler warnings", since they brought a lot of troubles and little
benefit. I also don't like that you changed files support/cpp/c-ppoutput.c
and support/cpp/libcpp/lex.c, which are just copies of GCC's cpp, which
make merging from GCC to SDCC mode difficult. The golden rule "If It's Not
Broken, Don't Fix It!" is valid for most cases, specially if you don't
exactly know what and why you are doing ;-)

In order to get rid of "ignore return values" warnings you can just
replace fgets(...) calls with (void) fgets(...). At least I think this
should help...

Which compiler (version) are you using? Any special command line options?

Borut

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=2472264&group_id=599