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

fix some error on mips. #69508

Closed
wants to merge 1 commit into from
Closed

Conversation

ClarkWang-2013
Copy link

@ClarkWang-2013 ClarkWang-2013 commented Feb 27, 2020

It mainly fixes two errors:

  1. occurs when build:
    /cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.6.2/src/raw/mod.rs:1071:(.text.ZN95$LT$hashbrown..raw..RawTable$LT$T$GT$$u20$as$u20$core..iter..traits..collect..IntoIterator$GT$9into_iter17hd24b8e25407575e5E+0x68):relocation truncated to fit: R_MIPS_CALL16 against `_Unwind_Resume@@GCC_3.0'

  2. Fix getrandom is not correct on the current kernel.

? @nikomatsakis

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2020
@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-27T08:09:49.4724289Z ========================== Starting Command Output ===========================
2020-02-27T08:09:49.4743760Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/87370e11-e72a-45d2-98bd-85012b571184.sh
2020-02-27T08:09:49.7436971Z 
2020-02-27T08:09:49.7507789Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-27T08:09:49.7544768Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69508/merge to s
2020-02-27T08:09:49.7561117Z Task         : Get sources
2020-02-27T08:09:49.7561604Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-27T08:09:49.7562314Z Version      : 1.0.0
2020-02-27T08:09:49.7562681Z Author       : Microsoft
---
2020-02-27T08:09:52.4720938Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-27T08:09:52.4901189Z ##[command]git config gc.auto 0
2020-02-27T08:09:52.4940899Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-27T08:09:52.4971996Z ##[command]git config --get-all http.proxy
2020-02-27T08:09:52.5065167Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69508/merge:refs/remotes/pull/69508/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@xen0n
Copy link
Contributor

xen0n commented Feb 29, 2020

Firstly, don't mix two irrelevant changes in one PR. Better separate them.

The link error is discussed in #52108. -mxgot fixes the problem but incurs a slight performance penalty, as all accesses through GOT need a longer sequence. From the inactivity of the linked issue it seems further discussion is needed regarding this.

As for the getrandom "fix", I don't think it's needed on upstream kernels, because the only circumstance -EINVAL could turn up is when the incoming flags parameter is invalid. However the flag is GRND_NONBLOCK so there's no way to hit that code path. What's special about your environment so as to need the adaptation?

@ClarkWang-2013
Copy link
Author

Firstly, don't mix two irrelevant changes in one PR. Better separate them.

The link error is discussed in #52108. -mxgot fixes the problem but incurs a slight performance penalty, as all accesses through GOT need a longer sequence. From the inactivity of the linked issue it seems further discussion is needed regarding this.

As for the getrandom "fix", I don't think it's needed on upstream kernels, because the only circumstance -EINVAL could turn up is when the incoming flags parameter is invalid. However the flag is GRND_NONBLOCK so there's no way to hit that code path. What's special about your environment so as to need the adaptation?

My system is based on mips64el architecture, Sys_GetRandom got 314; But mips64 standard got 313

@xen0n
Copy link
Contributor

xen0n commented Feb 29, 2020

My system is based on mips64el architecture, Sys_GetRandom got 314; But mips64 standard got 313

Then you're using a kernel, not only with out-of-tree patches, but outright ABI-incompatible with upstream. For no reason should upstream code support such a case, nor can any independent party (including the Rust team, but MIPS is tier-2 so this doesn't apply) verify that the fix indeed works and continues to work in the future.

Carry the getrandom adaptation downstream, on your own; meanwhile somehow fix the borked kernel!

@xen0n
Copy link
Contributor

xen0n commented Feb 29, 2020

For the record, OP's explanation is misleading, as Linux on both mips64 and mips64el have the getrandom syscall at 5000 + 313 (n64). It seems someone back-ported getrandom incorrectly, or delibrately sabotaged ABI compatibility, and never admitted nor fixed their mistake.

It's not system integrators' responsibility to cover up for someone else's incompetence, let alone upstream.

@wzssyqa
Copy link
Contributor

wzssyqa commented Feb 29, 2020

My system is based on mips64el architecture, Sys_GetRandom got 314; But mips64 standard got 313

The customers of your OS deserve sympathy.

@ClarkWang-2013
Copy link
Author

It's a real headache!!!

@xen0n
Copy link
Contributor

xen0n commented Feb 29, 2020

A suggestion to you, if you absolutely must support the crap kernel at your hand, if you can't modify the kernel to do the right thing.

Just DO NOT make the mismatched syscall ever; unconditionally fallback is the way to go. Otherwise, like in this PR, you're effectively invoking one syscall, whatever sits on number 5313 instead of the intended getrandom, with garbage arguments, with unknown side-effects. Which any sane person never wants.

I suppose, based on my experience with the Loongson ecosystem, the target distribution containing the faulty kernel should be in complete control of your company and/or a few others. And the packages are built with in-house automation too (or manual work, but it doesn't matter). So carrying the fix yourself should pose minimal risk and maintenance burden, as that area of Rust is comparatively lightly touched. Include the patch in your SRPM or equivalent package is enough.

Note that, after all this struggle, packages pulling their libc from crates.io are not affected, thus still broken on your system. This time it's impossible to fix things, though, because you can't modify the published sources of crates, and any such change like in this PR is not upstreamable because the condition doesn't exist in upstream Linux.

@xen0n
Copy link
Contributor

xen0n commented Feb 29, 2020

Or you could provide the sources of the faulty kernel, which we actually expect to be available due to Linux being GPL-licensed, and we can possibly work out some patch kernel-side to alleviate the problem.

@ClarkWang-2013
Copy link
Author

Or you could provide the sources of the faulty kernel, which we actually expect to be available due to Linux being GPL-licensed, and we can possibly work out some patch kernel-side to alleviate the problem.

There's no source code. It's not released by us. Otherwise, it will be changed

@xen0n
Copy link
Contributor

xen0n commented Feb 29, 2020

There's no source code. It's not released by us. Otherwise, it will be changed

It's rather sad to hear this. It is a real shame for that other company to violate GPL so blatantly. If we could know exactly what code sits on syscall 5313, it might be possible to special-case inputs likely coming for getrandom and correctly -ENOSYS on that, satisfying downstream assumption. As the kernel is already broken beyond repair, adding yet another kludge is certainly okay.

However, no matter how much sympathy the community could provide, it doesn't change the fact that things like these are not upstreamable. Those kernel people are people too, get this fact and recommendation through and they may as well listen. That's about all we can do.

@nikomatsakis
Copy link
Contributor

OK, I don't really know anything about which syscall maps to what numbers or whatever, but I have to say that I don't like the hostile tone I see in the comments here. @xen0n, for example, I don't think that saying "crap kernel" is really necessary, even if the kernel is incompatible with existing conventions.

I'm not 100% sure why I was tagged to review in any case as this sort of thing is not my specialty. I'm not sure who is maintaining the MIPS support, I don't' really have a strong opinion on these particular changes.

I do agree that it seems like the two changes in this PR could be broken into independent PRs, as they seem to be independent of one another (and clearly at least somewhat controversial).

@Dylan-DPC-zz
Copy link

@ClarkWang-2013 closing this as it seems controversial. Better to split this into separate PRs as per the suggestion above. Thanks

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked-closed and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2020
@jyn514 jyn514 added S-blocked Status: 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: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants