-
Notifications
You must be signed in to change notification settings - Fork 156
GCC Compatibility #787
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
GCC Compatibility #787
Conversation
…related to json ser/de with LLVMAliasSet and LLVMBasedICFG that were not detected untio now, because of nlohmann's implicit conversions feature
| static constexpr auto isLLVMZeroValue = [](const llvm::Value *V) noexcept { | ||
| return V == getInstance(); | ||
| return isZeroValueHelper(V); |
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.
What's the underlying reason to have a lambda here instead of a normal static member?
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.
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.
Hmm, not 100% sure here (github code review might hide things), but would this not just work in the same way with a static function?
struct Foo {
static int foobar(int val) {
return val * 2;
}
};
template <typename Fn>
int call(Fn&& f) {
return f(21);
}
int main() {
return call(Foo::foobar);
}
If I missed something pls let me know. Then we could try to find a way to express this better.
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.
It would work, but then foobar would implicitly decay into a function-pointer, whereas a lambda has its own type.
This has two implications: (1) A function-pointer requires storage space when captured by a flow/edge function -- the captureless lambda plays well with [[no_unique_address]] and empty-base optimization. (2) The function-pointer is less likely to be inlined.
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.
I just noticed that we can simplify the lambda using our recently-added fn template:
- static constexpr auto isLLVMZeroValue = [](const llvm::Value *V) noexcept {
- return isZeroValueHelper(V);
- };
+ static constexpr auto isLLVMZeroValue = fn<isZeroValueHelper>;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.
I'm not sure if the lambda gets completely removed in this usage context but I also don't have strong opinion on the usage here, my primary goal was just to get rid of the gcc hackaround.
If the fn fixes it, fine for me.
|
|
||
| static constexpr llvm::StringLiteral LLVMZeroValueInternalName = "zero_value"; | ||
| static bool isZeroValueHelper(const llvm::Value *V) noexcept { | ||
| // Need this helper function to make gcc happy |
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.
What's the problem here for gcc?
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.
For gcc, the type LLVMZeroValue is incomplete in the lambda, so it cannot perform the implicit conversion to pointer-to-base for the comparison V == getInstance()
include/phasar/PhasarLLVM/DataFlow/IfdsIde/Problems/IDEInstInteractionAnalysis.h
Outdated
Show resolved
Hide resolved
include/phasar/PhasarLLVM/DataFlow/IfdsIde/Problems/IDEInstInteractionAnalysis.h
Outdated
Show resolved
Hide resolved
* Make phasar compile with gcc (g++-11) and uncover (and fix) two bugs related to json ser/de with LLVMAliasSet and LLVMBasedICFG that were not detected untio now, because of nlohmann's implicit conversions feature * Address review comments * Simplify isLLVMZeroValue according to the discussion in secure-software-engineering#787
* added test coverage for ci * merged coverage job into build, removed all but one step for testing * test code for meeting * Call-Graph Improvements (#785) * minor fix in CHA and RTA resolvers * Several small improvements for call-graph resolving * Fix out-of-bounds access in getVarTypeFromIR() * Add address-taken functions caching in base resolver * Some cleanup in resolvers * pre-commit * Fix bug in the overloads of buildLLVMBasedCallGraph() that takes a CallGraphAnalysisType * Dbg-Info based Type Matching (#791) * moved stripPointerTypes() + debug prints * MetadataKind approach * I think we don't get around using getName() * unittests still fail * bugfix + cleanup * Some cleanup * Implement allocated-types collection in terms of debug info * Fix libdeps * Remove some unneeded includes --------- Co-authored-by: Fabian Schiebel <52407375+fabianbs96@users.noreply.github.com> Co-authored-by: Fabian Schiebel <fabian.schiebel@iem.fraunhofer.de> * Some fixes in the IterativeIDESolver (#782) * Fix the IterativeIDESolver by allowing to analyze problems that override extend() and combine() * Use the logger in the IterativeIDESolver * Make it possible to solve the IDEFeatureTaintAnalysis with the IterativeIDESolver * Fix errors introduced by merge * Bump dependencies (#784) * Remove boost from BitVectorSet (#788) * Remove boost from BitVectorSet * Minor deboostifying * Apply review comment on Bit-index-inbounds check + simplify operator<< * GCC Compatibility (#787) * Make phasar compile with gcc (g++-11) and uncover (and fix) two bugs related to json ser/de with LLVMAliasSet and LLVMBasedICFG that were not detected untio now, because of nlohmann's implicit conversions feature * Address review comments * Simplify isLLVMZeroValue according to the discussion in #787 * backup for rebase * cmake works, g++ fails * working version (with local hacks -20) * added regex for filter, it is not working * fixed filtering + cleanup * basic ci impl, lib coverage, fixed release cmake error * added cmake-scripts to submodule index * set llvm-cov before code-cov script * fixed version of llvm-cov * testing v19 due to ci issues * testing cov only when in debug build type * fixed all build type checks * added -v to see invocation of error in ci * only run coverage when matrix.build is Debug * Debug code + potential bug found? * only run cov when PHASAR_DEBUG_LIBDEPS is off * removed debug code, added messages for cov paths * testing llvm cov 19 * hardcoded llvm-19 for cov * adding llvm-19 as dependency for cov tools * disabling asan for release for now * removed unneccesary char * development merge + json external * Is the sanitizer the problem? * fixed cmake failing when in debug w/o -DCODE_COVERAGE=ON * added coverage report artifact uploading * fixed ci * fixed filtering not working correctly on runner * docker sanitizer back ON + new matrix entry DebugCov * Docker sanitizer still fails, disabling for now * Removed coverage of unittests * cleanup * Some cleanup * Add demangling to cov report * artifact should be entire all-merged/ folder * external diff * removed rename of index.html * Revert json downgrade --------- Co-authored-by: Fabian Schiebel <52407375+fabianbs96@users.noreply.github.com> Co-authored-by: Fabian Schiebel <fabian.schiebel@iem.fraunhofer.de> Co-authored-by: Fabian Schiebel <fabianbs@mail.upb.de>
Make phasar compile with gcc (g++-11 and g++-13) and uncover (and fix) two bugs related to json ser/de with LLVMAliasSet and LLVMBasedICFG that were not detected until now, because of nlohmann's implicit conversions feature.