as incorrectly handles immediate in "addu reg, reg, immediate"
alexfru opened this issue · 12 comments
I had this in assembly code generated by Smaller C:
addu $2, $2, 45904
GNU as turned it into:
7f0080e0: 3401b350 li at,0xb350
7f0080e4: 00411021 addu v0,v0,at
Our as did this instead:
7f0080e0: 2442b350 addiu v0,v0,-19632
Simple test to reproduce:
# cat >addiu.c
unsigned n = 0x123;
int main(void)
{
printf("n = 0x%08X\n", n);
asm("la $2, n");
asm("lw $3, 0($2)");
asm("addu $3, $3, 0xC000");
asm("sw $3, 0($2)");
printf("n = 0x%08X\n", n);
}
# libexec/smlrc addiu.c addiu.s
# cc addiu.s -o addiu
# ./addiu
n = 0x00000123
n = 0xFFFFC123
Expected output:
n = 0x00000123
n = 0x0000C123
I'm guessing, other instructions can be similarly affected. "subu reg, reg, imm" is affected.
Since addu $t,$s,imm is not a real instruction but a macro there's no telling what the "best" way of doing it is.
addiu $t,$s,imm is perfectly valid as long as imm is correct. I guess the problem here isn't the fact that it's using addiu but that the immediate value is being supplied as a signed value instead of an unsigned one, which I guess is the fault of the macro expansion in the assembler.
By the way, addiu is: Adds a register and a sign-extended immediate value and stores the result in a register
It looks like li is a macro as well since it's not listed as an instruction, so that may expand out to something like ori $t, $zero, imm which is guaranteed to treat the immediate value as a raw binary value instead of a signer or unsigned arithmetic value. As a macro it may even expand it further to:
lui $t, %hi{imm}
ori $t, $t, %lo{imm}I know, it's a macro. addiu behaves the same as addu in this case, I checked. The value of the immediate simply isn't examined here.
if ((type & FRD1) && (type & FRS2)) {
/* Three-operand instruction used with literal operand.
* Convert it to immediate type. */
unsigned newop;
switch (opcode & 0xfc0007ff) {
case 0x00000020: newop = 0x20000000; break; // add -> addi
case 0x00000021: newop = 0x24000000; break; // addu -> addiu <------ HERE IT CHANGES THE OP-CODE
case 0x00000024: newop = 0x30000000; break; // and -> andi
case 0x00000025: newop = 0x34000000; break; // or -> ori
case 0x0000002a: newop = 0x28000000; break; // slt -> slti
case 0x0000002b: newop = 0x2c000000; break; // sltu -> sltiu
case 0x00000022: newop = 0x20000000; // sub -> addi, negate
negate_literal = 1; break;
case 0x00000023: newop = 0x24000000; // subu -> addiu, negate
negate_literal = 1; break;
case 0x00000026: newop = 0x38000000; break; // xor -> xori
default:
uerror ("bad rt register");
return;
}
ungetlex (clex, cval);
cval = (opcode >> 11) & 31; /* get 1-st register */
newop |= cval << 16; /* set 1-st register */
newop |= opcode & (31 << 21); /* set 2-nd register */
opcode = newop;
type = FRT1 | FRS2 | FOFF16 | FMOD; // <------ HERE IT CHANGES THE TYPE TO IMMEDIATE
goto foff16;
}It's doing a simple 1:1 op-code replacement. It should really be doing it in a more complex and signed number friendly way really. The replacement op-code has a third operand type of FOFF16 - that is /* 16-bit relocatable offset */ It uses the getexpr() function to get the value to use for the third operand.
I don't know if any of that is of any help...
Yep, I've seen that place. All such ALU instructions truncate the immediate to 16 bits:
unsigned n = 0x123;
int main(void)
{
printf("n = 0x%08X\n", n);
asm("la $2, n");
asm("lw $3, 0($2)");
asm("addu $3, $3, 0xC000");
asm("sw $3, 0($2)");
printf("n + 0xC000 = 0x%08X\n", n);
asm("la $2, n");
asm("lw $3, 0($2)");
asm("subu $3, $3, 0xC000");
asm("sw $3, 0($2)");
printf("n - 0xC000 = 0x%08X\n", n);
asm("la $2, n");
asm("lw $3, 0($2)");
asm("ori $3, $3, 0x1111C000");
asm("sw $3, 0($2)");
printf("n | 0x1111C000 = 0x%08X\n", n);
static char s1[] = "a";
static char* p1 = s1 + 32768;
static char* p2 = s1 + 65536;
printf("s1 @ 0x%08X, s1+32768 @ 0x%08X, s1+65536 @ 0x%08X\n", s1, p1, p2);
n = 0;
printf("n = 0x%08X\n", n);
asm("la $2, n");
asm("lw $3, 0($2)");
asm("xori $3, $3, 0x11111111");
asm("sw $3, 0($2)");
printf("n ^ 0x11111111 = 0x%08X\n", n);
}
Output:
# ./addiu
n = 0x00000123
n + 0xC000 = 0xFFFFC123
n - 0xC000 = 0x00000123
n | 0x1111C000 = 0x0000C123
s1 @ 0x7F00C42B, s1+32768 @ 0x7F01442B, s1+65536 @ 0x7F01C42B
n = 0x00000000
n ^ 0x11111111 = 0x00001111
So, it looks like there's a chunk of code missing.
Yeah, it should really be loading the whole value into a register (lui + ori) then working with that value - that way it deals with the entire 32-bits, not just the lower 16-bits.
Well, it's actually a mismatch in expectations. GCC generally avoids using macro instructions (except LI and LA). To keep assembler compact, I intended to implement only absolutely required features. I understand that for SmallerC or manually written code it would be better to have some reasonable set of macros supported. Fixed in 51a8fd3
Yeah, see another code comment, there's incorrect range check.
Done.
There's still a minor defect. ".set at" doesn't appear to be the default setting (I've sent you an e-mail about it earlier). Because of that one can get an error from as when assembing something like "addu $2, $2, 33333" since the constant is too big and the at register is disallowed, but required.