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

Add saw-script llvm_struct_type function #1085

Closed
brianhuffman opened this issue Feb 19, 2021 · 3 comments · Fixed by #1088
Closed

Add saw-script llvm_struct_type function #1085

brianhuffman opened this issue Feb 19, 2021 · 3 comments · Fixed by #1088
Labels
easy Issues that are expected to be easy to resolve and might therefore be good for new contributors subsystem: crucible-llvm Issues related to LLVM bitcode verification with crucible-llvm

Comments

@brianhuffman
Copy link
Contributor

We have a function llvm_struct_value : [SetupValue] -> SetupValue for creating LLVM values in saw-script, but we do not have a corresponding function llvm_struct_type : [LLVMType] -> LLVMType for creating LLVM struct types. We should add it.

Note that we do already have a function called llvm_struct that gives you an LLVMType, but it only takes a string as an argument and gives you the corresponding alias type (which is often, but not necessarily, defined as a struct type).

The llvm_struct_type function will be specifically useful when you have a program that has defined a variable-length struct (i.e. one where the last field is an array with unspecified length, which is compiled to LLVM with an array length of 0) and you want to allocate one with a non-zero array length in the last field.

For completeness, we should probably also add a variant for packed struct types too, although these are rarely used in practice.

@brianhuffman brianhuffman added easy Issues that are expected to be easy to resolve and might therefore be good for new contributors subsystem: crucible-llvm Issues related to LLVM bitcode verification with crucible-llvm labels Feb 19, 2021
@RyanGlScott
Copy link
Contributor

Note that we do already have a function called llvm_struct that gives you an LLVMType, but it only takes a string as an argument and gives you the corresponding alias type (which is often, but not necessarily, defined as a struct type).

Wow, that's pretty confusing. I wonder if we should use a name like llvm_alias instead for this?

@brianhuffman
Copy link
Contributor Author

brianhuffman commented Feb 19, 2021

One problem with renaming llvm_struct to llvm_alias is that llvm_struct is already in use in a lot of places, so renaming it would break a bunch of existing proof scripts.

I suppose we could always just introduce a new name llvm_alias, and then phase out the old name later.

@RyanGlScott
Copy link
Contributor

I suppose we could always just introduce a new name llvm_alias, and then phase out the old name later.

Right, this is what I was thinking. Having llvm_struct coexist with llvm_struct_type seems ripe for confusion.

RyanGlScott added a commit that referenced this issue Feb 22, 2021
Having both `llvm_struct` and `llvm_struct_type` invites potential confusion,
and moreover, `llvm_alias` is a more accurate name anyways. See the discussion
in #1085.
RyanGlScott added a commit that referenced this issue Feb 22, 2021
This introduces the `llvm_alias` function and makes `llvm_struct` a synonym for
`llvm_alias`. For now, `llvm_struct` continues to be available without a
deprecation notice. In the future, we may want to deprecate `llvm_struct` in
favor of `llvm_alias`, as having two separate functions named `llvm_struct` and
`llvm_struct_type` could invite confusion. See the discussion in #1085.
@mergify mergify bot closed this as completed in #1088 Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Issues that are expected to be easy to resolve and might therefore be good for new contributors subsystem: crucible-llvm Issues related to LLVM bitcode verification with crucible-llvm
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants