Page MenuHomePhabricator

[stack-safety] Refactoring StackSafetyAnalysis to be accsessible from other passes
Needs ReviewPublic

Authored by gilang on Mon, Jun 24, 1:04 AM.

Details

Summary

Refactoring StackSafetyAnalysis to expose information about params and allocas outside of its compilation unit. Previously, the analysis does not expose params and allocas information externally and only provides print method for the user. In order to use the information programmatically from other passes, the data structure has to be exposed in the header files under llvm namespace, instead of anonymous namespace.

No modification of the original algorithm, therefore, it can utilize the original test scripts for testing.

Diff Detail

Event Timeline

gilang created this revision.Mon, Jun 24, 1:04 AM

Hi gilang, out of curiosity what is your intended use of the StackSafetyAnalysis? Is it necessary to expose all of the information exposed here?

Hi gilang, out of curiosity what is your intended use of the StackSafetyAnalysis? Is it necessary to expose all of the information exposed here?

Hi, thanks for the response.

We are currently planning to utilize StackSafetyAnalysis for memory protection schemes that continue our prior work on ARM pointer authentication (https://arxiv.org/abs/1811.09189).

Since the code seems to be dormant around 6 months, and I haven’t found recent discussion regarding this, I thought that I can suggest some refactoring so other users can use this analysis in other passes, similar with the rest of LLVM analyses suites (e.g., alias analysis). This is also to avoid changing the StackSafetyAnalysis code for every uses by different users, which will be redundant. However, if some other approach there is already an internal development to improve its API that the public currently does not know of, this changes can be dropped.

Right now we plan to use the exposed objects to identify stack-allocated variable which has out-of-bound access and use the information in our memory protection scheme.

PS: Apologize for double posting via e-mail. Seems that the email reply is not automatically added to the comment here. I replied via e-mail to also add a team member without Phabricator account to the loop. :)

eugenis added inline comments.Tue, Jun 25, 1:48 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
127

I'm not sure I understand this comment. Are the overloads for the new types shadowing the common overloads? Would it help to move them out of the anonymous namespace?

Btw, please upload change with full context, it's hard to review otherwise.
See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

gilang updated this revision to Diff 206591.Wed, Jun 26, 12:03 AM

Updating the patch with full context. Forgetting to add -U argument.

gilang marked an inline comment as done.Wed, Jun 26, 12:24 AM
gilang added inline comments.
llvm/lib/Analysis/StackSafetyAnalysis.cpp
127

Hi. Apologize for the inconvenience. I have updated the patch to include the full context.

So, without adding the using statement inside the block, the code will not compile due to overloading failure. Putting the operator overload declaration outside the anonymous namespace also does not help.

Clang used: 6.0.0-1ubuntu2

gilang marked an inline comment as not done.Wed, Jun 26, 12:25 AM
ish added a subscriber: ish.Thu, Jun 27, 7:58 AM

If that works for your use case, perhaps a better interface would be a query function that returns offset range or even bool is_safe given an AllocaInst* ?

llvm/lib/Analysis/StackSafetyAnalysis.cpp
127

I'm not sure where the problem is, but there are no such "using ::operator<<" declarations anywhere else in llvm, so there must be a way around it.

If that works for your use case, perhaps a better interface would be a query function that returns offset range or even bool is_safe given an AllocaInst* ?

I initially thought the same as it is more useful and more similar with the pattern used in alias analysis, but since the internals uses SmallVector to store the information, it would be inefficient if the user also need to iterate every AllocaInst* in their own pass, since the querying will takes O(n) complexity. I believe refactoring the SmallVector to, for instance, DenseMap, will require more code changes.

My first thought was to minimize the change to the original code so exposing the iterator to the structure was the faster choice, as the user can also access other information in the struct such as size. However, I will try to implement it as you suggested.

gilang updated this revision to Diff 207258.Mon, Jul 1, 3:38 AM

Revising the proposal to create a query function instead, by refactoring some internals to use DenseMap instead of SmallVector and provide isSafe(AllocaInst*) and getSizeOf(AllocaInst*) function.

eugenis added inline comments.Mon, Jul 8, 5:12 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
149

This could be very expensive - AllocaInfo is large, and DenseMap starts with 64 buckets. I wonder if SmallMapVector would work better here.