-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 diagnostic for stack allocations of 1 GB or more #119798
base: master
Are you sure you want to change the base?
Conversation
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fd9f9a6
to
8ee58b2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9fb4f4f
to
91dcd42
Compare
This comment has been minimized.
This comment has been minimized.
I have addressed all the comments, but not sure how to add the stdout file for 32 bit :
|
@rustbot review |
|
This comment has been minimized.
This comment has been minimized.
@iSwapna any updates on this? thanks |
Sorry, I am taking a class @Stanford CS103, taking every bit of my time outside of work. That's towards wanting a good grounding on CS/compilers as well, to do a better job here! There was a CI failure (fluent related) which I have not got an answer on how to address, the Zulip discussion is here: Issue 83060 - Regression with large stack arrays (2-4GB) If I could get an answer on how to address the CI issue, I can take care of this in a couple of weeks (after impending midterm) |
Sure, Thanks for the update. Just wanted to know if you are still working on it and have any updates. You should get a reply on zulip else you can bump it i guess |
will do! Thank you for checking! |
3305a06
to
96c95df
Compare
This comment has been minimized.
This comment has been minimized.
51786dd
to
34c0ab8
Compare
This comment has been minimized.
This comment has been minimized.
ed5919e
to
adbf2cc
Compare
please make sure the commit history is squashed as well 😀 |
adbf2cc
to
404c578
Compare
This comment has been minimized.
This comment has been minimized.
@rustbot review |
@@ -234,6 +236,14 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | |||
let layout = start_bx.layout_of(fx.monomorphize(decl.ty)); | |||
assert!(!layout.ty.has_erasable_regions()); | |||
|
|||
if layout.size.bytes() >= MIN_DANGEROUS_SIZE { | |||
let (size_quantity, size_unit) = human_readable_bytes(layout.size.bytes()); | |||
cx.tcx().dcx().emit_warn(errors::DangerousStackAllocation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if we are going to warn, perhaps should this be a lint so that people can suppress it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me figure how to add that- do you have a book pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will wait for help on the CI issue before adding more code and then having to revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can converting it to a lint be a new follow up PR? I can add an issue. This case should be a fairly rare warning ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be preferable to make this a lint in this PR. Having warnings appear that we cannot disable is not a good user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, could you please point me to an example I can follow?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bfd54d1
to
55f2a65
Compare
This comment has been minimized.
This comment has been minimized.
55f2a65
to
2f820d5
Compare
use ilog2 Update compiler/rustc_codegen_ssa/messages.ftl Co-authored-by: Michael Goulet <[email protected]> Run test only on 64 bit
d9232a0
to
72aa684
Compare
Add diagnostic for stack allocations of 1 GB or more
Zulip conversation [here](Issue 83060 - Regression with large stack arrays (2-4GB))
Do I generate an ICE or issue a warning?
cc #83060