Fix ui constant tests for big-endian platforms

[uweigand]

A number of tests under ui/const-ptr and ui/consts are currently failing on big-endian platforms as the binary encoding of some constants is hard-coded in the stderr test files.

Fix this by a combination of two types of changes:

• Where possible (i.e. where the particular value of a constant does not affect the purpose of the test), choose constant values that have the same encoding on big- and little-endian platforms.

• Where this is not possible, provide a normalize-stderr-test rule that transforms the printed big-endian encoding of such constants into the corresponding little-endian form.

Fixes part of #105383.

[rustbot]

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

[RalfJung]

I think I am fine with the general approach, but it seems very fragile. Without a big-endian CI runner this is bound to break again soon, in particular since there is zero documentation that even explains why we use such odd constants, or why we have these normalizing rules.

[uweigand]

I think I am fine with the general approach, but it seems very fragile. Without a big-endian CI runner this is bound to break again soon, in particular since there is zero documentation that even explains why we use such odd constants, or why we have these normalizing rules.

I can certainly add some comments to why the constants are chosen - there is actually already some precedent like:

// Use `0` as constant to make behavior endianness-independent.

in ub-enum.rs.

As to CI, I agree - it would be great to have CI running on a BE system. I'd be happy provide access to a s390x machine and/or set up CI infrastructure myself. Not sure if this would be integrated into the main CI machinery or run stand-alone - whatever the Rust community prefers.

Taking a different approach - the problem is really just the "raw byte" dumps, which are provided as extra note diagnostics. Maybe it would make sense to have some more generic normalize-stderr-test pattern that just strips out these hex dumps completely? That still should leave the core purpose of those tests in place ... [ Or else, a compiler switch to turn off those raw byte notes in the first place, which the tests could use? ]

[uweigand]

Taking a different approach - the problem is really just the "raw byte" dumps, which are provided as extra note diagnostics. Maybe it would make sense to have some more generic normalize-stderr-test pattern that just strips out these hex dumps completely? That still should leave the core purpose of those tests in place ...

For reference, I've now updated the patch to follow this approach, which significantly reduces the amount of changes needed to the test cases, and should make it much less fragile. This basically consists of adding:

+// Strip out raw byte dumps to make comparison endianness-independent:
+// normalize-stderr-test "([0-9a-f][0-9a-f] [0-9a-f][0-9a-f] )+ *│.*" -> "HEX_DUMP"

to all affected test case file, and updating the expected outputs.

The only change apart from this is to src/test/ui/const-ptr/forbidden_slices.rs, which exposes an endian-specific difference in this diagnostic output:

       constructing invalid value at .<deref>[0]: encountered 0x11, but expected a boolean

depending on which byte of D0 is not a boolean value (0 or 1). Fixed this by choosing a value of D0 that differs from 0 or 1 in all bytes.

What do you think?

[uweigand]

FYI @Mark-Simulacrum just pointed me to this alternative solution that I wasn't aware of: #104135. Either way would work for me ...

[RalfJung]

Taking a different approach - the problem is really just the "raw byte" dumps, which are provided as extra note diagnostics. Maybe it would make sense to have some more generic normalize-stderr-test pattern that just strips out these hex dumps completely? That still should leave the core purpose of those tests in place ... [ Or else, a compiler switch to turn off those raw byte notes in the first place, which the tests could use? ]

We want to have tests covering that output though. There is a bunch of subtle logic for the pointer printing and things like that which needs testing.

What we could do is to disable the raw byte dump in the regular const tests (or use aggressive normalization that mostly removes the raw byte dumps), and then have little-endian-only tests that check the raw byte dumps. I'd be fine with such an approach.

But I don't like the approach this PR takes right now, since it drastically reduces the test coverage of our raw byte printing.

As to CI, I agree - it would be great to have CI running on a BE system. I'd be happy provide access to a s390x machine and/or set up CI infrastructure myself. Not sure if this would be integrated into the main CI machinery or run stand-alone - whatever the Rust community prefers.

I don't think third-party runners are a possibility. This would have to somehow run on our existing runners, e.g. via qemu. The infra team would know if we have the CI capacity for that.

FYI @Mark-Simulacrum just pointed me to this alternative solution that I wasn't aware of: #104135. Either way would work for me ...

The problem with that is -- how is a contributor supposed to be able to update those reference files? Basically nobody will be able to just --bless the big-endian files. We already do this for 32bit and 64bit and it is annoying but bearable since at least 64bit Linux hosts can easily --bless both of these.

[uweigand]

What we could do is to disable the raw byte dump in the regular const tests (or use aggressive normalization that mostly removes the raw byte dumps), and then have little-endian-only tests that check the raw byte dumps. I'd be fine with such an approach.

I've tried to implement that approach by adding a new raw-bytes.rs test that collects those tests with raw byte dumps. Not sure if this matches what you were thinking of ... This can probably be improved upon; in particular, if we only care about the formatting of the dumps, raw-bytes.rs likely now contains redundant tests and could be simplified.

The problem with that is -- how is a contributor supposed to be able to update those reference files? Basically nobody will be able to just --bless the big-endian files. We already do this for 32bit and 64bit and it is annoying but bearable since at least 64bit Linux hosts can easily --bless both of these.

It seems to me it should be possible to "bless" files for another target simply via a cross-compiler. I tried a straightforward approach using --target, but this is more difficult than expected, because the infrastructure will always attempt to cross-build the std crate first - and the test cases actually do import (a few things from) std, so this might well be necessary. Unfortunately, the full std build depends on other crates like compiler_builtins and profiler_builtins, which in turn require a full C cross-toolchain, which may be nontrivial to provide on the host system.

This seems a pity, as those tests likely do not actually use anything from those special crates, in particular since they are compile-time tests anyway. I'm wondering if there couldn't be a way to enable cross-target compile-time tests using some form of a stripped-down std crate that can be built solely using the existing Rust compiler (which is already able to cross-compile)?

[RalfJung]

It seems to me it should be possible to "bless" files for another target simply via a cross-compiler.

You mean for check-pass tests, since we don't have to actually link/run them? Yeah that is true in principle, but in practice our cross-build support needs a target toolchain pretty early, so a bunch of work would be required to make that happen. Also having to run test --bless more often still takes increasing amounts of time and work (unless we find a way to say "bless all output files please" without having to manually invoke x.py 3 or 4 times).

So I think for now this is a reasonable trade-off.

[RalfJung]

Can you check which of these files even still need stderr-per-bitwidth? I assume some of them will have bitwidth-independent output now.

You might have to also nomalize the alloc-id part of the alloc dumps for that, but that seems worth the trade-off to me.

[RalfJung]

LGTM, modulo ideally even making these tests bitwidth-independent.

@oli-obk should also approve though.

[uweigand]

Can you check which of these files even still need stderr-per-bitwidth? I assume some of them will have bitwidth-independent output now.

I had to be a bit more thorough in removing all raw byte dumps (including those with alloc-ids), and normalize size/align values in these output lines:

  = note: the raw bytes of the constant (size: 16, align: 8) {

With that done, indeed for most of the tests, the 32bit and 64bit output files are now identical and can be re-unified. Patch updated accordingly.

The two remaining tests where stderr-per-bitwidth is still needed are const-eval/ub-enum.rs and std/alloc.rs, in both cases because of differences of this type:

<    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .align.0.<enum-tag>: encountered 0x00000000, but expected a valid enum tag
---
>    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .align.0.<enum-tag>: encountered 0x0000000000000000, but expected a valid enum tag

[RalfJung]

We could normalize encountered 0x0+ to encountered $NULL to mitigate that?

As long as we still have some tests that ensure that we print these numbers with as many leading 0s as matches the bitwidths of the current target.

[uweigand]

We could normalize encountered 0x0+ to encountered $NULL to mitigate that?

It's not always exactly 0, it's sometimes other (small) integers. The difference is always the number of leading zeros though.

As long as we still have some tests that ensure that we print these numbers with as many leading 0s as matches the bitwidths of the current target.

Sure. I guess that could be done in another patch? Seems not directly related to this one.

[RalfJung]

I seem to recall we are normalizing away leading 0x00000000 somewhere already... not sure.

Sure. I guess that could be done in another patch? Seems not directly related to this one.

Sure, fair. Thanks for getting rid of some of the stderr-per-bitwidth, that already helps!

[oli-obk]

@bors r+ rollup

This is a good step, let's land it and iterate in follow ups

[bors]

📌 Commit 73e7207 has been approved by oli-obk

It is now in the queue for this repository.