-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix invalid LLVM IR as detected by tests #2316
Conversation
949c407
to
0c88fca
Compare
@@ -2658,9 +2658,14 @@ AllocaInst *CodegenLLVM::getMultiMapKey(Map &map, | |||
key, |
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.
IIRC CreateGEP creates the right type based on its arguments so if we have to cast offset_val
the type of key_type
must be wrong?
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.
IIUC, key_type
is a byte array (to comprise all possible key types) so we have to cast the result of the GEP to the type that we want to store. The question here is if the cast to i64 *
is correct, as the extra key may not always be an i64
(although currently it always is).
Btw. there's a bug in this loop - offset
is never incremented so it won't work with multiple extra_keys
(at this point, at most one extra key is passed). @lenticularis39 perhaps you could fix that as a part of this 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.
The extra key is currently only used in histogram maps, where it is always i64
. I guess we can cast to extra_key->getType()->getPointerTo()
to accommodate for possible future uses of extra key.
As of the missing offset
incrementation: the extra keys are passed as a vector of Value
s, whose type has to be inferred from DataLayout
(unlike the type of non-extra keys). extra_keys_size
only contains the sum of the sizes of the extra keys (which is now the same as the size of the extra key, since there is only one used). Do you think it would be better to pass a vector of sizes instead of extra_keys_size
or to drop the argument altogether and infer the sizes from DataLayout
?
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.
Let's infer it from data layout, if possible.
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.
OK, I pushed both changes into the PR branch.
0c88fca
to
890fab2
Compare
Fixes various invalid LLVM IR errors.
The argument of the path helper is not always a pointer; frequently args structure is used, whose member access expression generates an integer rather than a pointer. This is solved by adding an inttoptr (or bitcast, if the argument is of a pointer type different than int8 *) instruction if needed.
Remove integer extension cast on the result of a pointer dereference operation. This cast caused a bug, since the type of the expression is always the pointer value type, not int64 (unlike integer dereferences).
Remove integer extension to int64 of result value of context access, since the value is not always 64-bit, in which case it would result in an invalid store instruction.
Increment offset when handing extra keys in CodegenLLVM::getMultiMapKey and infer their size from DataLayout. The total size of extra keys is now also inferred from DataLayout and the extra_keys_size argument previously used for it is removed. Currently this has no effect, since multiple extra keys are never used.
890fab2
to
f55985e
Compare
good stuff! |
With the option to verify LLVM IR generated by runtime tests (implemented in #2298), several issues with codegen were found. This PR includes commits to fix them, along with enabling LLVM IR verification in tests and enabling remaining failing LLVM 14 tests. The reasoning behind each fix is explained in the individual commits.
Closes #2297
Checklist
man/adoc/bpftrace.adoc
and if needed indocs/reference_guide.md
CHANGELOG.md