Visitor refactor + function linkage fix #173
Conversation
alessandrod
force-pushed
the
visitor-refactor
branch
from
460ec0a to
2eb9b5a
Compare
Don't use CStr to wrap MDString since MDStrings are not null terminated. They have an explicit length, and for our purposes we can assume that they're always valid utf8 since we're dealing with rust symbol names.
alessandrod
force-pushed
the
visitor-refactor
branch
2 times, most recently
from
ce52d6b to
e008ce4
Compare
|
🤝 Looking now. |
There was a problem hiding this comment.
Just tested this with my very complicated BPF program (using callbacks, tail calls, etc.) and no longer need the static linkage monkeypatch. I don't understand most of the LLVM code, but from user perspective, it looks good. Maybe some additional comments describing the linkage thinking process?
alessandrod
force-pushed
the
visitor-refactor
branch
from
e008ce4 to
34b2e4d
Compare
Thanks for looking and testing! Added some comments. |
|
I confirm that everything works when I compile my project with this version of the linker (tested with commit: 34b2e4d ) |
alessandrod
force-pushed
the
visitor-refactor
branch
from
34b2e4d to
fd68b9c
Compare
src/llvm/di.rs
Outdated
| }; | ||
|
|
||
| if let Item::Operand(operand) = &mut item { | ||
| // When we have an operand to replace, we must do so regardless of wheter we've already |
There was a problem hiding this comment.
The code is very neat, I find it a lot clearer than what it was (prior to the refactor). Nothing else to say than: thanks @alessandrod
src/llvm/di.rs
Outdated
| }; | ||
|
|
||
| if let Item::Operand(operand) = &mut item { | ||
| // When we have an operand to replace, we must do so regardless of wheter we've already |
There was a problem hiding this comment.
| // When we have an operand to replace, we must do so regardless of wheter we've already | |
| // When we have an operand to replace, we must do so regardless of whether we've already |
src/llvm/di.rs
Outdated
| Item::GlobalVariable(value) => *value, | ||
| Item::GlobalAlias(value) => *value, | ||
| Item::Function(value) => *value, | ||
| Item::FunctionParam(value) => *value, | ||
| Item::Instruction(value) => *value, | ||
| Item::Operand(Operand { value, .. }) => *value, | ||
| Item::MetadataEntry(value, _, _) => *value, |
There was a problem hiding this comment.
nit
| Item::GlobalVariable(value) => *value, | |
| Item::GlobalAlias(value) => *value, | |
| Item::Function(value) => *value, | |
| Item::FunctionParam(value) => *value, | |
| Item::Instruction(value) => *value, | |
| Item::Operand(Operand { value, .. }) => *value, | |
| Item::MetadataEntry(value, _, _) => *value, | |
| Item::GlobalVariable(value) | |
| | Item::GlobalAlias(value) | |
| | Item::Function(value) | |
| | Item::FunctionParam(value) | |
| | Item::Instruction(value) | |
| | Item::Operand(Operand { value, .. }) | |
| | Item::MetadataEntry(value, _, _) => *value, |
src/llvm/di.rs
Outdated
| Item::GlobalVariable(value) => *value as u64, | ||
| Item::GlobalAlias(value) => *value as u64, | ||
| Item::Function(value) => *value as u64, | ||
| Item::FunctionParam(value) => *value as u64, | ||
| Item::Instruction(value) => *value as u64, | ||
| Item::Operand(Operand { value, .. }) => *value as u64, | ||
| Item::MetadataEntry(value, _, _) => *value as u64, |
There was a problem hiding this comment.
Not blocking for this PR, but I' just wondering - since we have safe wrappers for the most of these types, couldn't we make them hashable and use these hashes instead of memory addresses?
Also, nit:
| Item::GlobalVariable(value) => *value as u64, | |
| Item::GlobalAlias(value) => *value as u64, | |
| Item::Function(value) => *value as u64, | |
| Item::FunctionParam(value) => *value as u64, | |
| Item::Instruction(value) => *value as u64, | |
| Item::Operand(Operand { value, .. }) => *value as u64, | |
| Item::MetadataEntry(value, _, _) => *value as u64, | |
| Item::GlobalVariable(value) | |
| | Item::GlobalAlias(value) | |
| | Item::Function(value) | |
| | Item::FunctionParam(value) | |
| | Item::Instruction(value) | |
| | Item::Operand(Operand { value, .. }) | |
| | Item::MetadataEntry(value, _, _) => *value as u64, | |
There was a problem hiding this comment.
Consider:
let value = <x>;
let i1 = Item::Operand(Operand { parent: ..., value, index: 1 });
let i2 = Item::Operand(Operand { parent: ..., value, index: 2 });
these are two different items, but same value. So we can't hash Item since it would be weird if eg we made the two items above hash to the same thing.
The thing we should do is hash Value. However since Value has a lifetime and DISanitizer doesn't yet, it's not easy. I've considered doing it, but to do it right we need a wrapper type for LLVMContext and then make everything borrow from it. It's a large change that is orthogonal to what's being done in this PR.
This is a refactor of the visitor to give it more structure, use less unsafe and generally make it a bit more idiomatic rust.
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:
Global function write() doesn't return scalar. Only those are supported.
verification time 18 usec
stack depth 0+0
...
The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.
In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.
This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.
Fixes aya-rs/aya#808
alessandrod
force-pushed
the
visitor-refactor
branch
from
fd68b9c to
f451f76
Compare
This improves the visitor and implements a fixup for the function linkage issue in aya-rs/aya#808 and aya-rs/aya#810 (comment).
See individual commit messages for more context.