Fix the recent breakage of the gba crate

[Lokathor]

As per this issue, I've added the required cfg attribute. I'm just going by what @parched said, I don't know if this is the perfect minimum cfg bound myself.

[parched]

You will need to add quotes around the features I believe, and thumb-mode probably needs to be added to https://github.com/rust-lang/rust/blob/c2ddf5a1dd54ebe18ffb794e096c28a4ed8e1d16/src/librustc_codegen_llvm/llvm_util.rs#L96

[Lokathor]

oh no! that's take a whole other PR first :/

[gnzlbg]

@Lokathor could you open an issue in rust-lang/rust upstream that missing double quotes here should raise an error ? Idk how easy / hard would that be to implement, but I doubt you are the first one to make this mistake.

[gnzlbg]

cc @japaric @Amanieu could any of you review this change ?

[Lokathor]

@gnzlbg I didn't put double quotes around the feature names (which is wrong) and then it failed to build (which is correct). That part of it seems fine as far as I can tell.

[parched]

BTW, a reference for this is under 'Architectures' http://infocenter.arm.com/help/topic/com.arm.doc.dui0473m/dom1361289881374.html

[parched]

@gnzlbg Do you think we could we add a thumbv4t target to CI? That's the most restricted target in terms of features.

[gnzlbg]

@parched sure, the thumb... targets are just built, so it should be easy to add them (just c&p-ing the couple of lines used for the other targets in .travis.yml).

[Lokathor]

Ah, I'm not sure how much the GBA deviates from any other "normal" ARMv4TDMI chip, but if we're adding targets to CI could we settle on a gba target file and then have that get into CI?

[Lokathor]

So here's a question: this can be fixed for GBA using just the existing "v6t2" feature bound, but then it'd stay broken for othes until the thumb-mode feature went in and we make a more precise bound.

Would it be acceptable to just have the partial fix and add an armv4t CI check, and do the rest later?

[gnzlbg]

but if we're adding targets to CI could we settle on a gba target file and then have that get into CI?

Sure, just add a build job that cargo install xargo, add a .json file to the ci/ folder for the target, and then just do xargo build --all --target=ci/my_target.json.

[Lokathor]

Cool. Any opinion on the other part about just doing the quick fix now and the rest later?

[gnzlbg]

Send a PR and we can explore it further, I’m in general in favor of incremental improvement, but It’ll depend on how exactly that looks like.

…

On Thu 14. Mar 2019 at 18:54, Lokathor ***@***.***> wrote: Cool. Any opinion on the other part about just doing the quick fix now and the rest later? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#704 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NpmjMw5yasmbHyItdbrcmonGEIm_Dks5vWozqgaJpZM4bn1s0> .

[Lokathor]

Cool, I'll update this PR when I get home

[Lokathor]

Agh, I misread it, there's no way to do a small change. I'll start a PR for adding "thumb-mode" to rustc I guess.

[gnzlbg]

You might want to open an issue and ping @parched, @Amanieu, and @japaric to make sure everyone is on the same page.

[gnzlbg]

So it seems that there are issues related to arm-mode and thumb2 :/

[Lokathor]

Near as I can tell, the CI for gba can't build because it first has to build core, which is depending on the code we're trying to patch.

@gnzlbg says that we should accept this as is, update rust-lang/rust, and then we can re-run CI and proceed from there. I agree. @ketsuban is the only other regular Rustacean that compiles for GBA and they've agreed that the target file is one we can settle on for here (and hopefully in to the tier-3 list in the main rust repo once this is sorted out).

[gnzlbg]

This PR "temporarily" disables support for APSR for all ARM targets.

I've merged this "as is" because I consider this addition a regression that broke downstream code, and there doesn't really seem to be a super-quick path to a good fix (need to support thumb-mode, arm-mode, etc.).

The current CI added here to track this fix is "broken" because xargo tries to build libcore which still contains the bug, making CI fail. This is a good thing because it means that the added CI catches this issue.

The next step is to update stdsimd upstream, and see if CI here starts passing.

Afterwards, we can work on taking the necessary steps for re-enabling APSR without any stress.

I'm sorry if this breaks some already-existing embedded Rust code relying on APSR via libcore. That code can temporarily continue to use APSR by using the core_arch crate from crates.io. I will hold on doing a new crates.io release until we have properly re-enabled the APSR intrinsic.

Since the code using APSR is very recent and has a good workaround available, while the code broken by the addition of APSR is old and lacks a workaround, I believe that proceeding in this way will cause the minimum amount of breakage. Temporary breakage on nightly seems, however, inevitable.