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

Add no_std support #126

Merged
merged 14 commits into from
Nov 1, 2018
Merged

Add no_std support #126

merged 14 commits into from
Nov 1, 2018

Conversation

XAMPPRocky
Copy link
Member

@XAMPPRocky XAMPPRocky commented Oct 4, 2018

This is a renewed version of #50 for rust-lang/#39503.

cc @Mark-Simulacrum

@XAMPPRocky XAMPPRocky changed the title Add no_std support [WIP] Add no_std support Oct 4, 2018
@XAMPPRocky XAMPPRocky force-pushed the master branch 3 times, most recently from 8ef7580 to 13371ce Compare October 5, 2018 14:12
@XAMPPRocky XAMPPRocky changed the title [WIP] Add no_std support Add no_std support Oct 5, 2018
@XAMPPRocky
Copy link
Member Author

I've fixed the problems on linux. The failures in the CI seem unrelated to the PR.

@alexcrichton
Copy link
Member

Thanks for the PR! I fear though that the strategy of sprinkling feature = "std" wherever necessary makes the code pretty unmaintainable and harder to read. I think there's perhaps some simple fixes though?

  • The entire capture module should probably be removed in no_std mode, only the bare-bones stack-based API is probably all we need to provide.
  • With locking not being possible, I think that some APIs need to be unsafe? I believe both libbacktrace and windows need to be synchronized to work safely
  • I think that we'll maybe want to pick a different abstraction for filenames if Path isn't available, because almost all platforms still want a file name one way or another
  • I think we still want Debug impls in no_std mode?
  • Getting the name of a symbol is pretty important, so I think we'll want a solution for that in no_std mode as well

@XAMPPRocky
Copy link
Member Author

@alexcrichton Thanks for the feedback, I currently have a in development version that addresses most of this. However I wanted to ask you what kind of abstraction were you thinking for the Path alternatives? We could have it return a &str? We could make this easily maintable by having a type BacktracePath = Path; on std and on core we have type BacktracePath<'a> = &'a str;?

@alexcrichton
Copy link
Member

I think Windows may be a stickler where the information is actually &[u16] instead of &[u8], so we can probably just add a custom enum that's something like ByteOrWideString and has convenience accessors when std is enabled

@XAMPPRocky XAMPPRocky force-pushed the master branch 2 times, most recently from c9ee9b4 to 5a4f807 Compare October 11, 2018 10:56
@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented Oct 11, 2018

@alexcrichton Okay, I've updated the PR with your feedback. Please let know if there's anything else required!

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think CI is still broken though?

src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/symbolize/mod.rs Outdated Show resolved Hide resolved
src/symbolize/mod.rs Outdated Show resolved Hide resolved
src/symbolize/mod.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/backtrace/mod.rs Outdated Show resolved Hide resolved
@XAMPPRocky XAMPPRocky force-pushed the master branch 6 times, most recently from 4f5276e to eb1ebb4 Compare October 15, 2018 21:36
src/lib.rs Outdated Show resolved Hide resolved
src/symbolize/dbghelp.rs Outdated Show resolved Hide resolved
src/symbolize/dbghelp.rs Outdated Show resolved Hide resolved
src/symbolize/mod.rs Outdated Show resolved Hide resolved
src/symbolize/mod.rs Outdated Show resolved Hide resolved
src/symbolize/mod.rs Outdated Show resolved Hide resolved
@XAMPPRocky XAMPPRocky force-pushed the master branch 4 times, most recently from ee00643 to 474c342 Compare October 17, 2018 14:16
@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented Oct 20, 2018

@alexcrichton Okay, I fixed everything wrong with the CI, and made changes in respect to your feedback. Let me know if there is anything else!

@alexcrichton
Copy link
Member

I've been working through this but unfortunately ran out of time for the near future. I don't mind picking this up from here, but some things I've noticed are:

  • There's a number of warnings across various platforms to squelch
  • Windows and --no-default-features is having issues, needs to unconditionally depend on winapi, and needs to remove "std" from winapi. Additionally need to be added to CI
  • Windows uses Box::new but can use #[repr(align)] instead now
  • APIs need to be documented now that they require the std feature
  • resolve and trace should both be implemented in terms of *_unsynchronized
  • Overall the BytesOrWideString is still somewhat uncomfortable on Windows, but I think it's workable still? We won't want to shut off things on windows entirely, we'll want some sort of fallback that still works. I think this means some utf-16 decoder may be needed
  • The printing logic for symbol names still needs to work the same way in/out of std

@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented Oct 23, 2018

@alexcrichton I don't mind working on it. in relation to resolve and trace, resolve can be implemented with resolve_unsynchronizedbut trace causes smoke_test_frames to fail. Would you know why that is, and how to solve that?

@alexcrichton
Copy link
Member

@Aaronepower oh that's probably because the test is quite brittle and some constants in there need to change, although if that doesn't get it passing I can help dig in too

@alexcrichton
Copy link
Member

Ok I've pushed up all the changes that I think I'd like to see, now waiting on CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants