#2458 shift operation gives incorrect error and emits bad code

[Peter Dons Tychsen]

Hello SDCC team,

Found this in some of my code:

uint32_t global;
void func(void)
{
  uint8_t i=18;
  uint8_t bit;

  bit = (BOOL)((global & (((uint32_t)1) << i)) && TRUE);
  if(bit)
  {
      while(1);
  }

Compiling this gives: "unreachable code"

The reason is the "i" variable which is only 8 bit. However, as the "1" is already casted, this should not matter.

What is worse, if the "i" variable is not a constant (e.g. comes from a function call), the code can build, but bad code is emitted. All bits above 8 are broken.

I think there are some general rules for casts that do not work here.

Thanks,

/pedro

[Peter Dons Tychsen]

Oh crap. Running the tests again after the patch was applied failed this time (11 per target). Hmm, must have applied the patch incorrectly the first time before running the tests, or i forgot to clean before running them.

OK, the patch needs more work. Good thing you challenged me on that, or i probably would not have noticed it.Sorry about that! :-)

Thanks,

/pedro

[Ben Shi]

The following test succeeds in GCC but fails in SDCC.

/*
   a.c
 */

#include <testfwk.h>

int a = 0x1000;
char b;

void ww(void)
{
  char c;
  c = (0x1000 + a) && 1;
  b = c;
}

void testBug(void)
{
  ASSERT (b == 1);
}

[Peter Dons Tychsen]

Hi Ben,

That is very interresting. I think this is because the left to write propagation that i debugged is also missing left to write propagation for "&&" just like it did for "<<" and ">>".

But then again, i might be way off as this codebase is new to me, or maybe what i am doing is symptom fighting and not the actual root cause.

Thanks,

/pedro

[Ben Shi]

One more suggestion. You need to build the libraries and do the reg-test with the same sdcc executable .

Some times I made modification then "make all; make install" , only the sdcc executable was rebuilt but the libraries were not. And the reg-test passed. But it failed again with doing "make clean" first.

[Peter Dons Tychsen]

Ah, good point. That should probably be raised as a bug in the deps, which really should catch that automatically. I the tools change, all output is also invalid. Its probably a one-liner missing in some makefile (missing rule deps).

"make all; make install"

I don't think you need the "make install" part. The regression tests picks up tools 'n' libs from the paths above automatically. I would not make sense if an install was necessary as it would make it impossible for anyone on a large server to use it (uni/office setup) as they are likely not root. Having to install stuff to test it also sounds nasty and un-unix like.

[Ben Shi]

I thought current status is OK. Rebuilding the fully libraries costs too much time. Sometimes I just add a printf to debug the code and want a quick build.

[Peter Dons Tychsen]

Hi,

Rebuilding the fully libraries costs too much time
No really.

"make" builds everything
"make -C src" builds compilers, linkers, and assemblers but not libs.

So if you are building everything its because you are in the wrong dir.

So i still think that deserves a separate bug, as it is a bug in the libs makefile, which should depend on the tools that generate them.

Thanks,

/pedro

[Peter Dons Tychsen]

The following patch makes the deps work:

[Peter Dons Tychsen]

OK i scrapped my patch and tried to debugged it top down from the start again:

I now beleive that Ben was right from the start. It is related to the "&&" operator and not to the "<<" operater. My fix for the "<<" worked in a limited sense, but it was mearly a bad hack to undo the damage already done by the "&&" and has other sideeffects.

Here is what i think is happening:
Top down:

1) The tree starts at the left with the "unsigned char". OK.
2) "unsigned char" is propagated across the "=" to the right side. OK.
2) On the right side the top level tree is a "&&". "unsigned char" must not be set on left side here as that will cause the math needed to calculate this branch to be capped in size which can cause the entire express to fail.

I think when reaching logical operators such as "&&" and "||", the propagated type should be RESULT_TYPE_NONE, so the tree itself calculates the type needed to do the calculation. There is no way to know at that level how much that branch needs for calculating.

This seems to fix both cases (mine and the one Ben found).

I am currently running all tests with this new patch. Will post in a moment the results, to check for regressions and code size/speed.

[Peter Dons Tychsen]

OK ran all tests before/after.

Not only do all the tests pass, but the change actually generates faster code. :-)

For stm8 cycle count goes from 7780449 -> 7779211,
and size 2053111 -> 2053222.

Thats almost a 1% speed increase! Not bad!

The code size goes up 100 bytes, but that accounts for for incorrectly generated code, and seems reasonable.

Here is the 2 runs.

Thanks,

/pedro

[Peter Dons Tychsen]

Add here is with patch...

[Peter Dons Tychsen]

And here is the latest fix:

[Peter Dons Tychsen]

Please review/apply/reject

Thanks,

/pedro

[Peter Dons Tychsen]

Ah, my 1% was 0,01%. but still ... :-)

I updated the patch to be more effective. I found out that if we just propagate BOOLEAN always it gives better code compression, and frankly makes more sense all left side of a logical operator always is bool. Generating new patch and data....

[Ben Shi]

Which is your final patch ? Or what is your final solution? I am busy recently and will take a look next week.

[Peter Dons Tychsen]

Sorry Ben, it is coming. tests are still running. But it is looking good.
Code size shrinks, speed is increased and the bugs are fixed (both).

[Peter Dons Tychsen]

Here is the final patch and test, and the test results.

1) Code size shrinks for all except 8051 variants.
2) Speed is up for all except 8051 variants
3) Both cases pass now (the one i found and the one you found).

I am not sure why the 8051 targets do not like it. They rise marginally in size/cycles.

Thanks,

/pedro

[Philipp Klaus Krause]

Most ports have full bool support: Since the standard requires that the adress of a bool can be taken, it tkes up the same sace as char in memory.
The 8051 is speecial: It has support for bit variables, and bool is just mapped to bit there. This results in bool vairables taking less space in memory, but can result in higher code size for code using bool variables. It also means that for 8051, one canot take the address of a bool vairable, and one cannot use bool inside struct or union.

Philipp

P.S.: I guess it would be nice to have full bool support in 8051, as a separate data type from bit.
P.P.S.: Mangled keyword names to work around Sourceforge. Finding the true spelling of the keywords is left as an exercise to the reader.

[Peter Dons Tychsen]

I totally agree. Mapping boolean on top of 8051s internal bit memory made allot of sense back in the good old days and was the way i myself used to make C51 code containing boolean variables. Now that C99 has come along it is not so clear.

So the language type "bit" should really map internally to RETURN_TYPE_BIT and bool should map to RETURN_TYPE_BOOL with no automatic translation between the two. The "bit" can then not be dereferenced via a pointer, but the "bool" can. Optionally an option could re-instate the current behviour where it automatically maps bools on bits if someone still needs that. For a totally straight-forward C program with no target specific options used, the current system still makes allot of sense, so the limited memory of the 8051 is used better.

One thing is for sure: After 20 years of hacking every bit on the 8051 and thinking every time is the last, i still find myself back on it again and again. So making sure SDCC works well on C99/C11/CXX makes allot of sense, as the old bad boy simply will not die in our lifetime. And to be honest, i would miss it if it did.

Thanks,

/pedro

[Peter Dons Tychsen]

Maybe the new new trees generated hit an unsupported case in the 8051 backend?

[Ben Shi]

Thanks for the patch. I am busy recently and will try to merge it next week.

[Philipp Klaus Krause]

I unexpectedly found a bit of free time today and had a look at this. I applied a variant of the proposed patch in revision [r9489].

Philipp

[Peter Dons Tychsen]

OK, thanks Phillip!

Now almost all my code can run in SDCC! :-).

In the mean time i have found out why the 8051 port rises slightly in size for tests.
This is a bug in the 8051 port that handles booleans poorly in certain cases.

I actually found 2 new bugs while investigating it. Will create new bugs for these.

This one should be closed-fixed.

Thanks,

PS: i did consider the "!" op but it does not have a "left" tree branch, so it makes no difference as far as i can see. Is there a case i missed?

/pedro

[Peter Dons Tychsen]

I fileda bug for the 8051 problem: https://sourceforge.net/p/sdcc/bugs/2465/