latin1 encoding difference when using &'static str and String

[Dreistein]

Hi,
First of all, thank you for creating and maintaining this library!

I was debugging some strange bug today in our library and found a difference in the output between passing &'static str and String.
Consider this minimal example:

#[test]
fn test1() {
    let str = "nopqrstuvwxyz{|}~¡¢£¤¥¦§¨©";

    // encode
    let mut buf = vec![0; str.len()];
    encoding_rs::mem::convert_utf8_to_latin1_lossy(str.as_bytes(), &mut buf);
    println!("encoded buffer: {buf:?}");

    // decode
    let mut out = " ".repeat(buf.len() * 2);
    encoding_rs::mem::convert_latin1_to_str(&buf, &mut out);
    println!("decoded string: '{}'", out);

    assert_eq!(str, out.trim_end_matches(|c| c == '\0' || c == ' '));
}

which is OK:

running 1 test
test string::test::test1 ... ok

successes:

---- string::test::test1 stdout ----
encoded buffer: [110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 161, 162, 163, 164, 165, 166, 167, 168, 169, 0, 0, 0, 0, 0, 0, 0, 0, 0]
decoded string: 'nopqrstuvwxyz{|}~¡¢£¤¥¦§¨©          '


successes:
    string::test::test1

But when i change the static string reference to a heap allocated one:

#[test]
fn test2() {
    let str = String::from("nopqrstuvwxyz{|}~¡¢£¤¥¦§¨©");

    // encode
    let mut buf = vec![0; str.len()];
    encoding_rs::mem::convert_utf8_to_latin1_lossy(str.as_bytes(), &mut buf);
    println!("encoded buffer: {buf:?}");

    // decode
    let mut out = " ".repeat(buf.len() * 2);
    encoding_rs::mem::convert_latin1_to_str(&buf, &mut out);
    println!("decoded string: '{}'", out);

    assert_eq!(str, out.trim_end_matches(|c| c == '\0' || c == ' '));
}

The test fails:

running 1 test
test string::test::test2 ... FAILED

successes:

successes:

failures:

---- string::test::test2 stdout ----
encoded buffer: [110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 161, 162, 163, 164, 165, 166, 167, 168, 169, 165, 194, 166, 194, 167, 194, 0, 0, 0]
decoded string: 'nopqrstuvwxyz{|}~¡¢£¤¥¦§¨©¥Â¦Â§Â    '

thread 'string::test::test2' panicked at src/string.rs:303:5:
assertion `left == right` failed
  left: "nopqrstuvwxyz{|}~¡¢£¤¥¦§¨©"
 right: "nopqrstuvwxyz{|}~¡¢£¤¥¦§¨©¥Â¦Â§Â"
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs:697:5
   1: core::panicking::panic_fmt
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:75:14
   2: core::panicking::assert_failed_inner
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:448:17
   3: core::panicking::assert_failed
             at .rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:403:5
   4: string::test::test2
             at ./src/string.rs:303:5
   5: string::test::test2::{{closure}}
             at ./src/string.rs:290:11
   6: core::ops::function::FnOnce::call_once
             at .rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:253:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs:253:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    string::test::test2

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 14 filtered out; finished in 0.06s

Note that the reason why it fails is totally on me. Nonetheless it's
a) completely unexpected for me that you write to the other part of the buffer
b) it's strange that the function has different behavior depending on the memory location. At least that is what i think is happening.

BR

[CrazyboyQCD]

Some investigation here, &str literal is promoted as static symbol, and the address alignment of that differs from string on the heap, resulting in different execution paths in this code snippet

encoding_rs/src/ascii.rs

Lines 116 to 192 in d823a51

	let mut until_alignment = {
	// Check if the other unit aligns if we move the narrower unit
	// to alignment.
	// if ::core::mem::size_of::<$src_unit>() == ::core::mem::size_of::<$dst_unit>() {
	// ascii_to_ascii
	let src_alignment = (src as usize) & ALU_ALIGNMENT_MASK;
	let dst_alignment = (dst as usize) & ALU_ALIGNMENT_MASK;
	if src_alignment != dst_alignment {
	// Safety: bails early and ends up in the naïve branch where usize-alignment doesn't matter
	break;
	}
	(ALU_ALIGNMENT - src_alignment) & ALU_ALIGNMENT_MASK
	// } else if ::core::mem::size_of::<$src_unit>() < ::core::mem::size_of::<$dst_unit>() {
	// ascii_to_basic_latin
	// let src_until_alignment = (ALIGNMENT - ((src as usize) & ALIGNMENT_MASK)) & ALIGNMENT_MASK;
	// if (dst.add(src_until_alignment) as usize) & ALIGNMENT_MASK != 0 {
	// break;
	// }
	// src_until_alignment
	// } else {
	// basic_latin_to_ascii
	// let dst_until_alignment = (ALIGNMENT - ((dst as usize) & ALIGNMENT_MASK)) & ALIGNMENT_MASK;
	// if (src.add(dst_until_alignment) as usize) & ALIGNMENT_MASK != 0 {
	// break;
	// }
	// dst_until_alignment
	// }
	};
	if until_alignment + ALU_STRIDE_SIZE <= len {
	// Moving pointers to alignment seems to be a pessimization on
	// x86_64 for operations that have UTF-16 as the internal
	// Unicode representation. However, since it seems to be a win
	// on ARM (tested ARMv7 code running on ARMv8 [rpi3]), except
	// mixed results when encoding from UTF-16 and since x86 and
	// x86_64 should be using SSE2 in due course, keeping the move
	// to alignment here. It would be good to test on more ARM CPUs
	// and on real MIPS and POWER hardware.
	//
	// Safety: This is the naïve code once again, for `until_alignment` bytes
	while until_alignment != 0 {
	let code_unit = *(src.add(offset));
	if code_unit > 127 {
	// Safety: Upholds safety-usable invariant here
	return Some((code_unit, offset));
	}
	*(dst.add(offset)) = code_unit as $dst_unit;
	// Safety: offset is the number of bytes copied so far
	offset += 1;
	until_alignment -= 1;
	}
	let len_minus_stride = len - ALU_STRIDE_SIZE;
	loop {
	// Safety: num_ascii is known to be a byte index of a non-ascii byte due to stride_fn's invariant
	if let Some(num_ascii) = $stride_fn(
	// Safety: These are known to be valid and aligned since we have at
	// least ALU_STRIDE_SIZE data in these buffers, and offset is the
	// number of elements copied so far, which according to the
	// until_alignment calculation above will cause both src and dst to be
	// aligned to usize after this add
	src.add(offset) as *const usize,
	dst.add(offset) as *mut usize,
	) {
	offset += num_ascii;
	// Safety: Upholds safety-usable invariant here by indexing into non-ascii byte
	return Some((*(src.add(offset)), offset));
	}
	// Safety: offset continues to be the number of bytes copied so far, and
	// maintains usize alignment for the next loop iteration
	offset += ALU_STRIDE_SIZE;
	// Safety: This is `offset > len - stride. This loop will continue as long as
	// `offset <= len - stride`, which means there are `stride` bytes to still be read.
	if offset > len_minus_stride {
	break;
	}
	}
	}
	break;

And If find_non_ascii returns earlier here, it passes the test, though I'm not sure if this is the solution.

encoding_rs/src/ascii.rs

Lines 1839 to 1851 in d823a51

	// Safety: src, dst both point to two valid `usize`s each
	// Safety-usable invariant: Will return byte index of first non-ascii byte.
	#[inline(always)]
	unsafe fn ascii_to_ascii_stride(src: *const usize, dst: *mut usize) -> Option<usize> {
	// Safety: src safety invariant used here
	let word = *src;
	let second_word = *(src.add(1));
	// Safety: src safety invariant used here
	*dst = word;
	*(dst.add(1)) = second_word;
	// Relies on safety-usable invariant here
	find_non_ascii(word, second_word)
	}

[hsivonen]

Thank you for reporting and thank you for debugging. As a preliminary finding, it seems that the reason why this hasn't been caught in Firefox is that the x86_64 SIMD code path seems to work.