Wrong formatting with format_args

[werner-matthias]

Formating yields incorrect result.
I tried this code at a bare metal Raspberry Pi (no_std):

    let foo: u32 = 13;
    kprint::fkprint(format_args!("foo = {} = {:?} = {:x} = {:b}\n",foo,foo,foo,foo));

kprint::fkprint writes to the frame buffer using the Write trait.

I expected to see this output:

foo = 13 = 13 = d = 1101

Instead, the output is:

foo = 21 = 21 = d = 1101

Meta

I'm using rustc 1.22.0-nightly (dd08c30 2017-09-12) with xargo, i.e., core is newly built.

[werner-matthias]

Actually, it seems to be a more general problem with the alignment at ARM.
In libcore/fmt/num.rs, ptr::copy_nonoverlapping(lut_ptr.offset(d1), buf_ptr.offset(curr), 2);
fails to address the correct bytes within DEC_DIGITS_LUT.

Since everything was okay a couple of weeks ago, there must be a breaking change in the meantime.
I could imagine several places where such change might happened: at the llvm (thus, I would have to update the target description), at the rust compiler, or indeed in the libcore. Could anybody give a hint?

[alexcrichton]

@werner-matthias this looks like it'll be pretty difficult for someone else to investigate (difficult to reproduce), do you think you can help narrow down the regression range if possible?

All target descriptions are in src/librustc_back/target, although this may or may not be related to that. A more narrow range for the regression would certainly help track this down!

[werner-matthias]

I could narrow it a bit: the change is somewhere around the 10th of September. Version ddd123e works, ca94c75 does not.

[alexcrichton]

Ok this is the range of commits between there. The only thing that looks even slightly suspect there is #44312, @eddyb do you know if that'd affect this?

Or maybe if there's bug with rvalue promotion?

[eddyb]

Rvalue promotion works a lot like regular statics, I fail to see a distinction.

@werner-matthias by "fails to address the correct bytes within DEC_DIGITS_LUT.", can you talk about exact addresses? It'd help to see what exact mismatch is occurring.
It's possible static was put in a different section or was accidentally having a higher alignment.

[werner-matthias]

From the patterns I observed, I would guess that there is an offset of -1. Thus, e.g., instead to address b"000102030405060708091011121314151617181... for 13 it comes up with b"000102030405060708091011121314151617181... .

[eddyb]

If it loads with 2-byte alignment that seems wrong libcore code, since b"..." is unaligned. Or is there a minimum alignment on that architecture that we're not respecting?

[werner-matthias]

ARM has a minimum alignment of 2 (unless it is explicitly deactivated).

[eddyb]

Ah that seems like something rustc doesn't know about. See #44440 - can you try setting that for your target?

[alexcrichton]

We just added support for global minimum alignment (#44440), but I'm surprised this hasn't come up before!

@werner-matthias is this global minimum alignment something we should set for all our ARM targets? Or just the one you're working on?

[werner-matthias]

IHMO llvm cares about alignment, if the target description is correct, especially the data layout. I use Japaric's target description for ARM.

[eddyb]

From what we've seen this extra property is not in the data layout.

[werner-matthias]

A wild guess: Could the introduced global minimum alignment be exactly the reason. since it possibly overwrites the architecture's defaults?

[eddyb]

It's not in that range though? My change is capable of breaking things that only worked by accident before.

[parched]

Hi @werner-matthias which raspberry pi 1,2 or 3? Could you please paste or link your exact target description?

[parched]

Oh actually I think I found it. Looks like raspi 1. You should also add the +strict-align feature unless you have set unaligned accesses allowed in SCTLR.

[werner-matthias]

@parched: You are right, +strict-align does the job also for ca94c75. Kudos!

[Mark-Simulacrum]

@eddyb I'm not entirely clear on whether this is a bug in Rust, so perhaps we can close?

[nikomatsakis]

triage: P-high

Seems like there is something wrong with (our?) target spec.

cc @japaric -- what should the target spec be?

[japaric]

Summary: @werner-matthias has been using this custom target specification and running on a bare metal ARMv6 environment (Raspberry Pi). They had to use the +strict-align option to force LLVM to generate only aligned access to memory. When they didn't they got undefined behavior / the problems described in the issue description.

The equivalent target that we support would be arm-unknown-linux-gnueabi(hf). Note that this is for a hosted (Linux) ARMv6 environment. We do not have the +strict-align option set in our target specs. However, I think this is not a problem because it seems that the Linux kernel enables unaligned accesses (I'm still looking for an official source about this). cf. comments by @werner-matthias and @parched below:

ARM has a minimum alignment of 2 (unless it is explicitly deactivated).

You should also add the +strict-align feature unless you have set unaligned accesses allowed in SCTLR.

As for the regression that @werner-matthias observed I think that may have been caused by the recent introduction of the "minimum global alignment" option to target specs. My guess is that since @werner-matthias's target spec does not have that option it's probably now defaulting to 1 whereas before llvm was providing it: it probably was 2. I need to confirm this.

[parched]

It wouldnt hurt enabling strict-align for arm-unknown-linux-gnueabi(hf), clang does for Armv6 Linux but not for Apple and BSD. I'm not sure if it's strictly required, at least for Linux on the raspi I'm pretty sure it's not. From memory there's some comments in the clang source about but I'm not sure how accurate and up to date they are. I'll try and find a link when I get a minute.

[japaric]

I found this document in the Linux kernel code -- I don't know how old / valid is though -- and it says that the kernel will (try to) fix unaligned memory access, at some cost, if the bit 1 of /proc/cpu/alignment is set. The exact text is quoted below:

The kernel will attempt to fix up the user process performing the unaligned access. This is of course slow (think about the floating point emulator) and not recommended for production use.

If that bit is set by default on all ARMv6 Linux devices then our current settings won't (or shouldn't) cause crashes. Though there's a performance penalty for having the kernel step in to fix the unaligned memory access.

It would hurt enabling strict-align for arm-unknown-linux-gnueabi(hf)

But would it hurt more than having the kernel fix things?

[japaric]

My guess is that since @werner-matthias's target spec does not have that option it's probably now defaulting to 1 whereas before llvm was providing it: it probably was 2. I need to confirm this.

Checking further this can't be the cause of the regression because (1) is not in the reported commit range and (2) if min-global-align is missing in the target spec we'll get the (right) alignment value from LLVM, which is what we have always been doing.

So no idea about the regression in the custom target spec.

[parched]

But would it hurt more than having the kernel fix things?

Sorry I meant to type wouldn't. So I agree we should add it.

[parched]

Here's the clang comment I was remembering.

Assume pre-ARMv6 doesn't support unaligned accesses.

ARMv6 may or may not support unaligned accesses depending on the
SCTLR.U bit, which is architecture-specific. We assume ARMv6
Darwin and NetBSD targets support unaligned accesses, and others don't.

ARMv7 always has SCTLR.U set to 1, but it has a new SCTLR.A bit
which raises an alignment fault on unaligned accesses. Linux
defaults this bit to 0 and handles it as a system-wide (not
per-process) setting. It is therefore safe to assume that ARMv7+
Linux targets support unaligned accesses. The same goes for NaCl.

The above behavior is consistent with GCC.

https://github.com/llvm-mirror/clang/blob/0810e2be8fa9a97519b8e3406e8f67645738c29b/lib/Driver/ToolChains/Arch/ARM.cpp#L462-L474

[cuviper]

The intention of #44440 is only to increase alignment, and only for global symbols, not memory access in general. Targets that don't set min-global-align should not see any effect at all.

[nikomatsakis]

Checking in from the @rust-lang/compiler meeting -- can someone summarize what action on our part is needed, if any? Or is that still undetermined?

[arielb1]

@japaric

I think this just used to work by accident before. Could you please open a PR for fixing the minimum alignment target spec?

[parched]

@nikomatsakis @arielb1 There's no bug in rustc here, the bug was in the out of tree target spec that was used. The take away from some of the discussion though is, we should probably add +strict-align to the in tree Armv6 targets for performance, not correctness, reasons. Do you agree @japaric?

[japaric]

we should probably add +strict-align to the in tree Armv6 targets for performance, not correctness, reasons

Yeah, I agree. The Cortex-M target already has that feature enabled for correctness (otherwise programs crash) but all the other ARM Linux targets should see perf improvements. I'd be interested in seeing an after vs before RPi benchmark -- just to get an idea of the perf "penalty" the current targets have.

[japaric]

PR #45094 adds +strict-align to all the ARMv6 targets