Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for x86 64 bit integer stack arguments. #2007
Add support for x86 64 bit integer stack arguments. #2007
Changes from 1 commit
9b2a33a
0d09633
ab915d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why the change from
integerToAlignment defaultStackBaseAlign
tonoAlignment
?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.
nice catch! Actually, this reveals a bug. The stack needs to be 16 byte aligned before the call instruction is executed, that is, before the return address has been pushed. The existing check is on
%rsp
after the return address has been pushed.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.
Are we only handling 64-bit arguments at this point? If
test_llvm_x86_08
changed argumenth
fromunsigned long
tounsigned char
, does clang promote it to a ulong argument or do we fail here?Also, what are the expected semantics of
fail
here? Do we have aMonadFail
instance with special handling, or should this use panic as on line 1061?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.
Longer term, it might be interesting to look at the
abide
library and utilize the functionality from there in this effort to consolidate and centralize our ABI handling. That said, I think that would be a future effort and not part of this utilitarian PR.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.
yes, this only handles 64-bit arguments. I didn't investigate what clang does, I think it doesn't promote it. The semantics if
fail
, since this can happen, and it's just not handled. Also, this doesn't handlefloat
/doable
, and other non-integer types.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.
Where do we detect/handle stack overflow? Is that just normal SAW detection of addressing memory not valid for the allocation in 969+970?
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.
yes, it's just standard detection.