Improved varargs handling #376
Conversation
|
The override registration mechanism in |
robdockins
force-pushed
the
varargs
branch
2 times, most recently
from
153b64e
to
7369b8a
Compare
|
Internal CI looks green, I think this is ready to merge. |
There was a problem hiding this comment.
I really like what you've done here, both in terms of making Crucible-the-language varargs-aware, and in the changes to override setup. 👍 I've certainly been bitten by writing non-matching LLVM and Crucible types for overrides before, I look forward to never dealing with that again.
I think the biggest things that stand to be improved are:
- Possible deduplication with
llvm-pretty- If this happens, the quasiquoter could also be upstreamed to that package
- Adding newtype for pointers with concrete allocation numbers
- Possibly upstreaming that one function to parameterized-utils.
All of the above are discussed in more detail in inline comments. I would like to see (1) and (3) resolved before merging, but not necessarily (2).
In the interest of transparency, I didn't check the following:
- The definitions of overrides in crux-llvm
- That you didn't change the type declarations of any overrides
I think we're pretty safe on (2) if tests are passing.
| import qualified Lang.Crucible.LLVM.Intrinsics.Common as IC | ||
| import Lang.Crucible.LLVM.Types | ||
|
|
||
| data QQType |
There was a problem hiding this comment.
Can we reuse llvm-pretty's Text.LLVM.AST.Type here? If not, it would be good to add a comment explaining what distinguishes this type from that one.
There was a problem hiding this comment.
The main difference is the addition of the QQVar and QQIntVar constructors, which represent the quasiquoter metavariable forms $var and #var. Indeed, probably it is work commenting this distinction.
| | QQOpaque | ||
| deriving (Show, Eq, Ord, Data) | ||
|
|
||
| data QQDeclare = |
There was a problem hiding this comment.
Similarly, can we reuse or discuss the differences with Text.LLVM.AST.Declare?
|
I'll open a ticket regarding the idea of generalizing/unifying the quasiquoter with the one currently in |
There was a problem hiding this comment.
Nice! I really like the approach you took to the review comments above, especially the new MalformedLLVMModule module.
I still haven't reviewed the mostly-copy/pasted sections. Let me know if you'd like me to do so, but I'm happy to trust that they're correct since the changes as you describe them are small.
This consists of an additional `FnVal` constructor that helps record how many of the tailing arguments at a function call site need to be packed into a vector. Then, at call resolution time, these arguments are reorganized into the expected form.
…d to resolve varagrs calls statically, at call sites, based on the claimed type of the function being called. However, clang doesn't actually respect the distinction between varargs and non-varargs functions, and assumes that the ABI for function calls is identical for varargs and non-varargs functions depending only on the number and type of actual arguments provided. In particular, it is happy to bitcast in such a way ignores the distinction, and treats partial external function specifications as though they are varargs. As a result, we must instead resolve varargs function calls at runtime. A previous commit added the minimal necessary support to core crucible. Here, we ensure that when we resolve a function pointer into a callable function, we do the necessary bookeeping so that the variable argument list can be properly packed as expected by the implementation.
so they get cached in the `LLVMContext` instead of being computed on demand.
reduce the amount of boilerplate required to implement intrinsics in crucible-llvm
and creates the associated function pointer in the memory at the time the intrinsic is installed. This in contrast to previous methods, where the handles will be installed a priori, based on the `declare` signature. Now, we instead allocate the handle based on the implementation signature provided with the intrinsic itself. This matters now that the signatures do not have to match exactly. To make this work, we had to refactor where `LLVMContext` is defined so that we can break a module cycle when we reference `allocLLVMHandleInfo` in `Intrinsics.Common`. Also, refactor the way that translation and global variable setup works to remove the antiquated `symbolMap` from the `LLMVContext` datastructure. The information that used to be contained in that map is now properly stored in the memory model, and the redundant map was confusing and could be out of sync with the canonical information in the memory. By finally delaying the binding of function pointers to function handles, we can correctly deal with the varargs/partial function signature issue. Make it easier to get crucible types from an LLVM function declaration.
override template mechanism used by crucible-llvm to register its intrinsics. To do this, we needed to update the `regiseter_llvm_intriniscs` function slightly so that downstream packages can participate in the intrinsic matching process.
Whitespace and minor fixes Co-Authored-By: Langston Barrett <[email protected]>
uses of `fail` and such with it when appropriate. There are still plenty of places where we should continue to use this more.
Apparently I need to update my wetware spellcheck dictionary Co-Authored-By: Langston Barrett <[email protected]>
Fixes #364