Skip to content
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 handling of points-to flags for memcpy in Steensgaard #373

Merged
merged 11 commits into from
Feb 1, 2024
Merged

Conversation

phate
Copy link
Owner

@phate phate commented Jan 28, 2024

  1. Fixes the handling of flags for memcpy in Steensgaard. The method ignored flags so far.
  2. Added a unit test that exposes the problem
  3. Some minor adjustments for debugging Steensgaard

Closes #334

@phate phate requested review from haved and sjalander January 28, 2024 12:54
Copy link
Collaborator

@haved haved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but isn't the flag copying one level too shallow? See comment

jlm/llvm/opt/alias-analyses/Steensgaard.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@haved haved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code still accidentally applies flags from the addresses. I was wondering if target location unification doesn't automatically merge the flags of locations pointing to the unified locations?

If a->x and b->y, with b being marked as pointing to escaped/external and x and y get unified, will that operation automatically mark a as pointing to escaped/external?

jlm/llvm/opt/alias-analyses/Steensgaard.cpp Outdated Show resolved Hide resolved
haved
haved previously approved these changes Jan 30, 2024
Copy link
Collaborator

@haved haved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the current framework of flags, this makes sense. I'm still a bit unsure about what happens when locations don't know about pointing to escaped until propagation, when you have

s -> a -> u
d -> b -> u

And then a turns out to escape later due to s being returned or otherwise leaving the module. Then u also escapes due to propagation, but b is not marked as pointing to escaped. Does b however need to be marked as escaping, or is it enough to have one pointer to u being marked as PointingToEscaped?

@phate
Copy link
Owner Author

phate commented Jan 30, 2024

I will overhaul this PR tomorrow. As written above, it currently is not properly working.

@phate
Copy link
Owner Author

phate commented Jan 31, 2024

@haved : I updated the memcpy analysis. Could you have a look at it again, please?

Copy link
Collaborator

@haved haved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this seems like a safe fix for handling late propagation. It would be interesting to see if this would continue to be an issue if the flags were on the pointee location, and not on the pointing location, but such a redesign might introduce other issues I haven't thought about

@phate phate merged commit b4f02a4 into master Feb 1, 2024
8 checks passed
@phate phate deleted the issue-334 branch February 1, 2024 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiling 557.xz with --AASteensgaardAgnostic results in error
2 participants