Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assuming that char is signed #4

Closed
mengstr opened this issue Feb 27, 2017 · 6 comments
Closed

Assuming that char is signed #4

mengstr opened this issue Feb 27, 2017 · 6 comments

Comments

@mengstr
Copy link

mengstr commented Feb 27, 2017

Hi, thanks for putting up this project at github. I've currently using it in a project running on a ESP8266 (Xtensa LX106 CPU) and ran in to some troubles with the code expecting that a char is signed in a few places.

The xtensa compiler assumes that a char is signed unsigned.

        int sum1=100; int sum2=100; int sum3=100;
        int value=-5;
        char c=(char)value;
        unsigned uc=(unsigned char)value;   
        signed char sc=(signed char)value;
        sum1+=c;
        sum2+=uc;
        sum3+=sc;
        os_printf("sum+c=%d sum+uc=%d sum+sc=%d\n",sum1,sum2,sum3);
        // ESP8266 --> sum+c=351 sum+uc=351 sum+sc=95
        // GCC X86 --> sum+c=95  sum+uc=351 sum+sc=95

This screwed up the implementation of relative jumps (something that zexall didn't detect!) and also made zexall report failures for the shift and rolls instructions. I spent a good while trying to figure out what what wrong with the shf/rol implementation, but when I by chance discovered the bug in the relative jumps I then realized what was the cause and it was an easy fix to get it all working perfectly.

Would you be interested in a pull request for my fix of these issues?

@allender
Copy link

allender commented Feb 27, 2017

Hello. I have verified that this is a problem on compiler that assume char is unsigned. I think that the OP meant to indicate that the xtensa compiler assumes char is unsigned. I compiled my project with char as unsigned by default and also ran into the same shift issues. Just wanted to confirm that this is indeed a problem.

image

@mengstr
Copy link
Author

mengstr commented Feb 27, 2017

Ah yes.... I got confused while writing this issue - sorry for that. :-) But you are understanding me correctly. The xtensa compiler have unsigned chars whereas the code assumes that chars are signed.

But my crc's was slightly different from yours:

<rlca,rrca,rla,rra>...........  OK
shf/rot (<ix,iy>+1)...........  ERROR **** crc expected:710034cb found:cb127686
shf/rot <b,c,d,e,h,l,(hl),a>..  ERROR **** crc expected:a4255833 found:b5821b2b
<set,res> n,<bcdehl(hl)a>.....  OK

@anotherlin
Copy link
Owner

Hi everyone,

Thank you for reporting this bug.
Yes, I indeed assumed that char are signed, but I can't remember why.
My mistake, standard doesn't specify if char is signed or unsigned by default.
I will fix that ASAP.

Best regards,

@anotherlin
Copy link
Owner

Hi again,

Ok, I reproduced the problem using gcc and "-funsigned-char" option.
Your CRCs are what I found, Mark's is CRC for zextest, and SmallRoomLabs' is CRC for zexall.
Long ago, I tried z80emu on an old SGI using MipsPro and I had a similar CRC error.
Don't remember well and didn't have time to investigate but I guess it was the same bug.
Working on fixing that, thanks again for your reports.

Best regards,

@anotherlin
Copy link
Owner

Hi everyone,

Ok, I've fixed the code. And tested with "-funsigned-char".
Can you guys please give it a try, thank you?
If that's fine, I'll close this issue and update the README.txt.
You'll be credited and thanked, of course.

Best regards,

@mengstr
Copy link
Author

mengstr commented Feb 28, 2017

Runs fine on myLX106 hardware now. You even found the RLC_INDIRECT_HL: which I had missed in my fixes. Strangely enough it seems like the zex'es doesn't cover that particular instruction fully enough to discover errors in the displacement.

Great job! Thanks for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants