Filtered AliasSet #723

fabianbs96 · 2024-05-10T12:31:27Z

The LLVMAliasSet contains a lot of spurious aliases due to its context insensitivity and its almost not present field-sensitivity.
Hence, the client analyses based on this alias information produce many false positives and need to propagate many more facts than necessary, resulting in a severe performance hit.

Therefore, some analyses in phasar, such as the IFDSTaintAnalysis and IDEExtendedTaintAnalysis, filter the alias sets on-the-fly to achieve better results.

This PR provides a wrapper FilteredLLVMAliasSet around the LLVMAliasSet that abstracts and caches the filtering.

vulder

Where do you implement the caching (I might have missed it 😆 ) and how exactly do you ensure that the cache is a) not to big and b) how do you do cache invalidation.

vulder · 2024-05-12T07:43:46Z

include/phasar/PhasarLLVM/Pointer/FilteredLLVMAliasSet.h

+
+class FilteredLLVMAliasSet {
+public:
+ using traits_t = AliasInfoTraits<FilteredLLVMAliasSet>;


you might call it alias_traits_t so it easier to see what kind of traits one accesses in the following code.

vulder · 2024-05-12T07:44:04Z

include/phasar/PhasarLLVM/Pointer/FilteredLLVMAliasSet.h

+ FilteredLLVMAliasSet(const FilteredLLVMAliasSet &) = delete;
+ FilteredLLVMAliasSet &operator=(const FilteredLLVMAliasSet &) = delete;
+ FilteredLLVMAliasSet &operator=(FilteredLLVMAliasSet &&) = delete;
+


Suggested change

vulder · 2024-05-12T07:44:13Z

include/phasar/PhasarLLVM/Pointer/FilteredLLVMAliasSet.h

+
+ FilteredLLVMAliasSet(const FilteredLLVMAliasSet &) = delete;
+ FilteredLLVMAliasSet &operator=(const FilteredLLVMAliasSet &) = delete;
+ FilteredLLVMAliasSet &operator=(FilteredLLVMAliasSet &&) = delete;


Suggested change

FilteredLLVMAliasSet &operator=(FilteredLLVMAliasSet &&) = delete;

FilteredLLVMAliasSet &operator=(FilteredLLVMAliasSet &&) noexcept = delete;

Does it make a difference to declare a deleted function noexcept?

Yes, a) you do the same thing for the move ctor and b) with noexcept is the correct signature of the function, so deleting it like this is the proper way

vulder · 2024-05-12T07:46:12Z

include/phasar/PhasarLLVM/Pointer/FilteredLLVMAliasSet.h

+private:
+ FilteredLLVMAliasSet(MaybeUniquePtr<LLVMAliasSet, true> AS) noexcept;
+
+ MaybeUniquePtr<LLVMAliasSet, true> AS;


Suggested change

MaybeUniquePtr<LLVMAliasSet, true> AS;

MaybeUniquePtr<LLVMAliasSet, true /*requiresAlignment*/> AS;

vulder · 2024-05-12T07:57:13Z

lib/PhasarLLVM/Pointer/FilteredLLVMAliasSet.cpp

+ return nullptr;
+}
+
+[[nodiscard]] static bool isConstantGlob(const llvm::GlobalValue *GlobV) {


Suggested change

[[nodiscard]] static bool isConstantGlob(const llvm::GlobalValue *GlobV) {

[[nodiscard]] static bool isConstantGlobalValue(const llvm::GlobalValue *GlobV) {

I find Glob not super easy to understand. I immediatly mixed it with shell globbing.

vulder · 2024-05-12T07:59:24Z

lib/PhasarLLVM/Pointer/FilteredLLVMAliasSet.cpp

+ if (const auto *Glob = llvm::dyn_cast<llvm::GlobalVariable>(GlobV)) {
+ return Glob->isConstant();
+ }


Why do you need a dyn_cast here? GlobV is already the type you want.

GlobV here is a GlobalValue, but I need it to be a GlobalVariable in order to call isConstant().

Ah, sry I miss read that

vulder · 2024-05-12T08:04:52Z

lib/PhasarLLVM/Pointer/FilteredLLVMAliasSet.cpp

+ if (llvm::isa<llvm::ConstantExpr>(Alias) ||
+ llvm::isa<llvm::ConstantData>(Alias)) {


Suggested change

if (llvm::isa<llvm::ConstantExpr>(Alias) ||

llvm::isa<llvm::ConstantData>(Alias)) {

if (llvm::isa<llvm::ConstantExpr, llvm::ConstantData>(Alias)) {

Nice, thanks; I didn't know about that!

fabianbs96 · 2024-05-12T13:06:03Z

Hi @vulder, thanks for your comments.
To answer your questions:

Where do you implement the caching (I might have missed it 😆 )

The caching is implemented in getAliasSet(), but now I have also implemented it for getReachableAllocationSites()

and how exactly do you ensure that the cache is a) not to big and b) how do you do cache invalidation.

Currently, I have not implemented cache invalidation and just keep the cache growing. In my tests (on some coreutils), it has not been any problem. Also, for IFDS and IDE analyses, it is hard to predict, when an alias set is never used again.

One could implement some ref-counting or least-recently-used strategy, but I havent' implemented it (yet?)

vulder · 2024-05-13T20:26:56Z

Hi @vulder, thanks for your comments. To answer your questions:

Where do you implement the caching (I might have missed it 😆 )

The caching is implemented in getAliasSet(), but now I have also implemented it for getReachableAllocationSites()

and how exactly do you ensure that the cache is a) not to big and b) how do you do cache invalidation.

Currently, I have not implemented cache invalidation and just keep the cache growing. In my tests (on some coreutils), it has not been any problem. Also, for IFDS and IDE analyses, it is hard to predict, when an alias set is never used again.

One could implement some ref-counting or least-recently-used strategy, but I havent' implemented it (yet?)

Ok, just keep in mind invalidation could also be interesting when the cache grows to big. So, even when a set would be needed again afterwards it would be useful to remove it from the cache to reduce the memory footprint.

MMory · 2024-05-16T11:48:43Z

lib/PhasarLLVM/DataFlow/IfdsIde/Problems/IFDSTaintAnalysis.cpp

@@ -155,7 +155,7 @@ void IFDSTaintAnalysis::populateWithMayAliases(
 container_type &Facts, const llvm::Instruction *Context) const {


The name Context is misleading/confusing me, context rather resembles a calling context, while here it is used as the statement at which the aliases are requested, in the sense of flow-sensitivity

fabianbs96 self-assigned this May 10, 2024

fabianbs96 requested review from pdschubert, vulder and MMory as code owners May 10, 2024 12:31

fabianbs96 force-pushed the f-FilteredAliasSet branch from 51ed40e to 6f2c0a2 Compare May 10, 2024 12:39

fabianbs96 added 2 commits May 10, 2024 14:42

Add alias set filter

d1891f5

Add test

52b24a0

fabianbs96 force-pushed the f-FilteredAliasSet branch from 6f2c0a2 to 52b24a0 Compare May 10, 2024 12:42

vulder requested changes May 12, 2024

View reviewed changes

fabianbs96 added 2 commits May 12, 2024 13:05

apply review comments from vulder

4e10792

Add caching for reachable allocation sites

e1e8fb5

fabianbs96 added 2 commits May 12, 2024 15:50

Bugfix

5ef3331

adding noexcept

f027b39

MMory approved these changes May 16, 2024

View reviewed changes

fabianbs96 and others added 3 commits May 18, 2024 12:11

Rename Context to AliasQueryInst in IFDSTaintAnalysis

9000760

Merge branch 'development' into f-FilteredAliasSet

5b31ecd

Merge branch 'development' into f-FilteredAliasSet

da93441

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filtered AliasSet #723

Filtered AliasSet #723

fabianbs96 commented May 10, 2024 •

edited

Loading

vulder left a comment

vulder May 12, 2024

vulder May 12, 2024

vulder May 12, 2024

fabianbs96 May 12, 2024

vulder May 13, 2024

vulder May 12, 2024

fabianbs96 May 12, 2024

vulder May 12, 2024

fabianbs96 May 12, 2024

vulder May 12, 2024

fabianbs96 May 12, 2024

vulder May 13, 2024

vulder May 12, 2024

fabianbs96 May 12, 2024

fabianbs96 commented May 12, 2024

vulder commented May 13, 2024

MMory May 16, 2024

	FilteredLLVMAliasSet &operator=(FilteredLLVMAliasSet &&) = delete;
	FilteredLLVMAliasSet &operator=(FilteredLLVMAliasSet &&) noexcept = delete;

	MaybeUniquePtr<LLVMAliasSet, true> AS;
	MaybeUniquePtr<LLVMAliasSet, true /requiresAlignment/> AS;

	[[nodiscard]] static bool isConstantGlob(const llvm::GlobalValue *GlobV) {
	[[nodiscard]] static bool isConstantGlobalValue(const llvm::GlobalValue *GlobV) {

		if (llvm::isa<llvm::ConstantExpr>(Alias) \|\|
		llvm::isa<llvm::ConstantData>(Alias)) {

	if (llvm::isa<llvm::ConstantExpr>(Alias) \|\|
	llvm::isa<llvm::ConstantData>(Alias)) {
	if (llvm::isa<llvm::ConstantExpr, llvm::ConstantData>(Alias)) {

		@@ -155,7 +155,7 @@ void IFDSTaintAnalysis::populateWithMayAliases(
		container_type &Facts, const llvm::Instruction *Context) const {

Filtered AliasSet #723

Are you sure you want to change the base?

Filtered AliasSet #723

Conversation

fabianbs96 commented May 10, 2024 • edited Loading

vulder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianbs96 commented May 12, 2024

vulder commented May 13, 2024

Choose a reason for hiding this comment

fabianbs96 commented May 10, 2024 •

edited

Loading