Merged
Conversation
…related to json ser/de with LLVMAliasSet and LLVMBasedICFG that were not detected untio now, because of nlohmann's implicit conversions feature
vulder
requested changes
Aug 20, 2025
Comment on lines
+55
to
+60
| static constexpr auto isLLVMZeroValue = [](const llvm::Value *V) noexcept { | ||
| return V == getInstance(); | ||
| return isZeroValueHelper(V); |
Collaborator
There was a problem hiding this comment.
What's the underlying reason to have a lambda here instead of a normal static member?
Member
Author
Collaborator
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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>;
Collaborator
There was a problem hiding this comment.
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 |
Collaborator
There was a problem hiding this comment.
What's the problem here for gcc?
Member
Author
There was a problem hiding this comment.
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
vulder
approved these changes
Oct 11, 2025
mxHuber
pushed a commit
to fabianbs96/phasar
that referenced
this pull request
Oct 17, 2025
* 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
fabianbs96
added a commit
that referenced
this pull request
Nov 8, 2025
* 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 <[email protected]> Co-authored-by: Fabian Schiebel <[email protected]> * 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 <[email protected]> Co-authored-by: Fabian Schiebel <[email protected]> Co-authored-by: Fabian Schiebel <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.