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

[MSP430] Build std components for msp430. #51250

Closed
wants to merge 3 commits into from

Conversation

pftbest
Copy link
Contributor

@pftbest pftbest commented May 31, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2018
@alexcrichton
Copy link
Member

@pftbest to confirm, do you have a good sense for how often this target breaks? If it's pretty rarely this seems fine by me to land, but if it's pretty common I think we may want to perhaps hold off on it for now

@pftbest
Copy link
Contributor Author

pftbest commented Jun 1, 2018

The last major break was in November 2017 when codegen units where enabled by default (#45836). It was fixed in January (#47453).

Since then there where two minor breaks, one in April (#49618) where #[cfg(target_pointer_width)] was missing, and one in May (#50369) where cast from char to usize was giving a warning.

In both cases the problem was the fact that msp430 is a 16bit bit target, and it is easy to forget that usize can be 16bit wide. For example in this pull request i have yet another fix for a similar problem. As you can see the fix was trivial in all this cases.

There is also one more thing you should know about, due to LLVM bug the build will fail when DEPLOY=1 flag is missing. It is set in CI so it will not fail the travis build, but it may be surprising if somebody tries to build dist-various-1 image locally. If you are not comfortable with this, I can try to fix LLVM assertion but I'm not sure how long it will take.

@TimNN TimNN added T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2018
@Mark-Simulacrum
Copy link
Member

We discussed this today in the infra meeting, and decided that we'd like to hold off for now on landing this until the LLVM assertion is fixed. DEPLOY doesn't actually fix the assertion, rather it just disables LLVM assertions, so we'd prefer to not land a target that is potentially/likely generating incorrect code. Does that sound reasonable to you?

@pftbest
Copy link
Contributor Author

pftbest commented Jun 5, 2018

DEPLOY doesn't actually fix the assertion, rather it just disables LLVM assertions

In this particular case the problem is caused by integer overflow checks that gets enabled when DEPLOY=0. Without this range checks libcore compiles fine even with LLVM assertions enabled.

But I understand your concern, so I'll see what I can do about it.

@pietroalbini pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2018
@pietroalbini
Copy link
Member

Ping from triage @pftbest! It's been a while since we heard from you, will you have time to work on this again?

@pftbest
Copy link
Contributor Author

pftbest commented Jun 25, 2018

Hello, I have no progress on this yet, sorry.

@bors
Copy link
Contributor

bors commented Jun 29, 2018

☔ The latest upstream changes (presumably #51569) made this pull request unmergeable. Please resolve the merge conflicts.

@TimNN TimNN added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 3, 2018
@TimNN
Copy link
Contributor

TimNN commented Jul 3, 2018

Ping form triage! I'm marking this as blocked (on the LLVM bug).

@TimNN
Copy link
Contributor

TimNN commented Jul 10, 2018

Ping from triage! Thanks for your PR, @pftbest. It looks like the blocking bug is not going to be resolved anytime soon, so we're closing this PR for now. Feel free to reopen in the future!

@TimNN TimNN closed this Jul 10, 2018
@TimNN TimNN added S-blocked-closed and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 10, 2018
@pftbest pftbest deleted the build_msp430 branch July 23, 2019 21:50
@cr1901
Copy link
Contributor

cr1901 commented Mar 22, 2020

@pftbest I think this might be worth trying to get merged again now:

  1. The LLVM bug you linked seems to be fixed.
  2. The last time I can recall the msp430 backend not emitting code period was 10 months ago.
  3. I'm unaware of any outstanding LLVM bugs (including ones that don't outright break code generation) since 2 months ago.
  4. If @pftbest, @awygle, or someone on @asl's (msp430 LLVM backend maintainer) team can get bug fixes and features upstream in LLVM, I'm on standby to quickly merge them into Rust's LLVM fork.
  5. Merging quickly into Rust LLVM's fork should be fine for now because we are stuck on nightly until at least Tracking issue for the msp430-interrupt calling convention/ABI #38487 is resolved.

@jyn514 jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked-closed labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants