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

Array comparisons don't work on AVR #78022

Closed
piegamesde opened this issue Oct 16, 2020 · 6 comments
Closed

Array comparisons don't work on AVR #78022

piegamesde opened this issue Oct 16, 2020 · 6 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AVR Target: AVR processors (ATtiny, ATmega, etc.) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@piegamesde
Copy link
Contributor

For some reason, comparing to identical arrays does not yield true in all cases.

I tried this code:

#![no_std]
#![no_main]

extern crate panic_halt;
use arduino_uno::prelude::*;

#[arduino_uno::entry]
fn main() -> ! {
	let peripherals = arduino_uno::Peripherals::take().unwrap();

	let mut pins = arduino_uno::Pins::new(
		peripherals.PORTB,
		peripherals.PORTC,
		peripherals.PORTD,
	);

	let mut serial = arduino_uno::Serial::new(
		peripherals.USART0,
		pins.d0,
		pins.d1.into_output(&mut pins.ddr),
		57600,
	);

	let a = [ true, true, true, true, true ];
	let b = a;
	ufmt::uwriteln!(&mut serial, "{}\r", a == b).void_unwrap(); // prints false

	let a = [ true, true, true, true ];
	let b = a;
	ufmt::uwriteln!(&mut serial, "{}\r", a == b).void_unwrap(); // prints true
	
	let a = [ 0, 1 ];
	let b = a;
	ufmt::uwriteln!(&mut serial, "{}\r", a == b).void_unwrap(); // prints false

	let a = [ 0 ];
	let b = a;
	ufmt::uwriteln!(&mut serial, "{}\r", a == b).void_unwrap(); // prints true

	loop {}
}

I expected to see this happen: all lines print true

Instead, this happened: some print false. See the code.

Meta

rustc --version --verbose:

rustc 1.49.0-nightly (dd7fc54eb 2020-10-15)
binary: rustc
commit-hash: dd7fc54ebdca419ad9d3ab1e9f5ed14e770768ea
commit-date: 2020-10-15
host: x86_64-unknown-linux-gnu
release: 1.49.0-nightly
LLVM version: 11.0

I am cross compiling for the avr-unknown-unknown target. I did not try any other targets except the host (x86_64-unknown-linux-gnu), on which it does work.

@piegamesde piegamesde added the C-bug Category: This is a bug. label Oct 16, 2020
@jonas-schievink jonas-schievink added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AVR Target: AVR processors (ATtiny, ATmega, etc.) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 16, 2020
@piegamesde
Copy link
Contributor Author

Any known workarounds? This is blocking one of my projects :(

@jonas-schievink jonas-schievink added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Nov 7, 2020
@piegamesde
Copy link
Contributor Author

I can still reproduce it on cargo 1.50.0-nightly (bfca1cd22 2020-11-24).

@drmorr0
Copy link
Contributor

drmorr0 commented Oct 30, 2021

I'm running into this issue as well. I know what's going on, but I don't have any idea how to fix it. The short version is that the compiler loads the address of the array into a register, calls memcmp, and then (for reasons I don't understand) tries to compare the value in that register (that's holding the array address) to 0. Which, obviously, isn't going to work. Here's the relevant assembly:

// Load address of memory region 1 into R25 and R24
00000092 ce.01                MOVW R24,R28		Copy register pair 
00000093 01.96                ADIW R24,0x01		Add immediate to word

// Load address of memory region 2 into R23 and R22
00000094 be.01                MOVW R22,R28		Copy register pair 
00000095 6a.5f                SUBI R22,0xFA		Subtract immediate 
00000096 7f.4f                SBCI R23,0xFF		Subtract immediate with carry

// Initialize the length of the array 
00000097 45.e0                LDI R20,0x05		Load immediate 
00000098 50.e0                LDI R21,0x00		Load immediate

// Call memcmp -- does not modify the contents of R22 and R23
00000099 0e.94.7b.01          CALL 0x0000017B		Call subroutine
0000009B e0.e0                LDI R30,0x00		Load immediate 
0000009C f0.e0                LDI R31,0x00		Load immediate

// Compare R22 (which still contains the address of memory region 2) with 0x00.  This clears the zero bit.
0000009D 60.30                CPI R22,0x00		Compare with immediate 
0000009E 70.07                CPC R23,R16		Compare with carry 

// Check the return value from memcmp
0000009F 8e.07                CPC R24,R30		Compare with carry 
000000A0 9f.07                CPC R25,R31		Compare with carry

// The zero bit is cleared so we never branch
000000A1 09.f0                BREQ PC+0x02		Branch if equal 
000000A2 10.e0                LDI R17,0x00		Load immediate 
000000A3 11.70                ANDI R17,0x01		Logical AND with immediate
...
0000017B fb.01                MOVW R30,R22		Copy register pair 
0000017C dc.01                MOVW R26,R24		Copy register pair 
0000017D 04.c0                RJMP PC+0x0005		Relative jump 
0000017E 8d.91                LD R24,X+		        Load indirect and postincrement 
0000017F 01.90                LD R0,Z+		        Load indirect and postincrement 
00000180 80.19                SUB R24,R0		Subtract without carry 
00000181 21.f4                BRNE PC+0x05		Branch if not equal 
00000182 41.50                SUBI R20,0x01		Subtract immediate 
00000183 50.40                SBCI R21,0x00		Subtract immediate with carry 
00000184 c8.f7                BRCC PC-0x06		Branch if carry cleared 
00000185 88.1b                SUB R24,R24		Subtract without carry 
00000186 99.0b                SBC R25,R25		Subtract with carry 
00000187 08.95                RET 		        Subroutine return 

memcmp operates in the following manner: it copies the values of R23-R22 into Z and R25-R24 into X; these are the addresses of the two arrays/memory regions to compare. R21-R20 contain the length of the memory region to compare. On each iteration of the loop, it loads the next byte from each memory region into R24 and R0, and then subtracts them (storing the result in R24), breaking the loop if R24 is non-zero. The return value from memcmp is in R25 and R24; if these registers are 0, then the two memory regions are equal, otherwise they are not.

Critically, note that R22 and R23 are not modified by memcmp at all.

When memcmp returns, the program clears Z and then compares R22 with 0 -- which, since it has not been modified at all, still contains the lower byte of the first memory region to compare, and is (almost) always going to fail.

So anyways, that's what's going on, but I have no idea why or how to fix it.

@drmorr0
Copy link
Contributor

drmorr0 commented Oct 30, 2021

On further investigation, I think the issue is in the LLVM IR:

%11 = call addrspace(1) i32 @memcmp(i8* nonnull %5, i8* nonnull %10, i16 5) #7, !dbg !1191
%.not = icmp eq i32 %11, 0, !dbg !1191 

I'm not an expert in LLVM IR by any stretch, but if I'm interpreting this correctly it's doing a 32-bit compare for the return value of memcmp. Moreover, this section of the code seems to indicate that LLVM thinks that memcmp is returning a 32-bit value:

; Function Attrs: nounwind optsize
declare i32 @memcmp(i8*, i8*, i16) local_unnamed_addr addrspace(1) #4    

@drmorr0
Copy link
Contributor

drmorr0 commented Oct 30, 2021

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 3, 2022
…rochenkov

make memcmp return a value of c_int_width instead of i32

This is an attempt to fix rust-lang#32610 and rust-lang#78022, namely, that `memcmp` always returns an `i32` regardless of the platform.  I'm running into some issues and was hoping I could get some help.

Here's what I've been attempting so far:

1. Build the stage0 compiler with all the changes _expect_ for the changes in `library/core/src/slice/cmp.rs` and `compiler/rustc_codegen_llvm/src/context.rs`; this is because `target_c_int_width` isn't passed through and recognized as a valid config option yet.  I'm building with `./x.py build --stage 0 library/core library/proc_macro compiler/rustc`
2. Next I add in the `#[cfg(c_int_width = ...)]` params to `cmp.rs` and `context.rs` and build the stage 1 compiler by running `./x.py build --keep-stage 0 --stage 1 library/core library/proc_macro compiler/rustc`.  This step now runs successfully.
3. Lastly, I try to build the test program for AVR mentioned in rust-lang#78022 with `RUSTFLAGS="--emit llvm-ir" cargo build --release`, and look at the resulting llvm IR, which still shows:

```
...
%11 = call addrspace(1) i32 `@memcmp(i8*` nonnull %5, i8* nonnull %10, i16 5) rust-lang#7, !dbg !1191                                                                                                                                                                                                                                %.not = icmp eq i32 %11, 0, !dbg !1191
...
; Function Attrs: nounwind optsize                                                                                                                                                                                                                                                                                          declare i32 `@memcmp(i8*,` i8*, i16) local_unnamed_addr addrspace(1) rust-lang#4
```

Any ideas what I'm missing here?  Alternately, if this is totally the wrong approach I'm open to other suggestions.

cc `@Rahix`
@drmorr0
Copy link
Contributor

drmorr0 commented Apr 4, 2022

This issue should be fixed at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AVR Target: AVR processors (ATtiny, ATmega, etc.) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants