-
Notifications
You must be signed in to change notification settings - Fork 11
Feature request: option to maintain or preserve rbp #144
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
Comments
This should be quite an easy feature. To do that, we need to add this argument in argParse.ts, and probably add it in |
Cool. I think it would be nice to match flag names to other compilers, or at least not do the exact opposite of them. I intend to return to this issue and see if I can hack up either of the new options. |
Cool.
|
Oh and just another thought, will Fiat's checker handle this accordingly? |
Sounds good. I think it does not matter whether local variables are addressed from rbp or rsp, though perhaps using both of them could get you twice the space with single-byte immediates. Fiat Crypto currently does not check anything about rbp but works fine if it is unchanged as long as the stack size is specified. |
I don't get this. Are you referring to the constant memory offset when referencing data on the stack like okay, we've recently had this where we are inferring the red zone by calling convention and the rest is inferred by usages of |
Yes, and I don't expect this to be important. What I was thinking of was that you could set rbp and rsp 256 bytes apart and then address the region between them from one or the other using single-byte immediates. I guess then the 128 bytes below rsp would also be usable on unix per red-zone rules. IIUC there is no convention for using only rbp, you always use rsp and then also rbp if that's required by the environment or the stack-frame size isn't known at compile time. |
I don't think it's worth setting things up like that, without indicators that this would bring performance benefits. |
Just to clarify, I understood that you wanted to implement this to get familiar with the code base, hence I did not get started on this. Now though, that we found the other two issues are much more work plus I still cant really reproduce the Debian segfaults, It might be easier, if I just implement this, if you don't object. :) |
Thank you. If you want this done asap, yes, please implement it. I do intend to return to this still, even if I'd have to be developing in docker. I'm interested in getting to know the codebase anyway and I want this feature; just haven't got to it yet. |
I've implemented a version of this in the I've attached the asm-files (as txt because github does not like asm) for the three options. |
I'm happy to also emit the cfi directives, if you can give me the syntax and when to emit those. |
This seems great! I followed https://www.imperialviolet.org/2017/01/18/cfi.html for CFI directives. Also feel free to use any BoringSSL asm file as an example. The idea is that no matter which instruction the program is paused it, it should be possible to recover the callee-saved registers by looking at the stack locations specified by previous CFI directives. |
I gave this a test drive and successfully recovered a couple of cycles. :) Merging would be neat regardless of whether CFI work is to follow soon. |
I've merged this into the |
closed with c1317f2 |
rbp
-based stack unwinding is a requirement for stack-sampling profiling in a security-conscious production deployments where debug information cannot be evaluated during sampling, e.g. Google-wide profiling.I imagine it would be pretty easy to just get CryptOpt to entirely ignore the
rbp
register by ripping it out from the list of registers. It'd be nice to add a command-line flag that achieves this behavior. Perhaps we could also have an option to update rbp as in-fno-omit-frame-pointer
so time spent in CryptOpt-generated routines does not get misattributed during profiling.Again, I'd be happy to do the work given the go-ahead and high-level guidance.
The text was updated successfully, but these errors were encountered: