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

Be more conservative on memory effects when args are mutable #52536

Closed
wants to merge 1 commit into from

Conversation

gbaraldi
Copy link
Member

So this is a start, but I'm still not satisfied about this, also the same treatment should probably be passed to setfield!. Though I'm not sure if it's also ever going to be valid. setfield! requires a mutable argument, so it's never going to be m.
The only way I can see this ever being refined if the types are already known is using escape analysis + getfield/setfield. Which I couldn't find if we do.

Fixes #52531

@vchuravy
Copy link
Sponsor Member

This is consistent with my analysis.

I don't fully understand the design of inaccesiblememonly I would have expected it to be w.r.t a single statement and not w.r.t the parent function.

E.g. getfield should always be argmemonly and never inaccessiblememonly. It is consistent if an only if the argument is function local (which we don't track just now?) and it does not alias a set field operation (when in a loop).

@aviatesk
Copy link
Sponsor Member

This modification might be overly conservative. The key insight from the issue is that QuoteNode can embed global variables that users can modify. I believe the right approach is to properly taint QuoteNode's :inaccessiblememonly (which we're already doing for GlobalRef).

In the current design of the effect analysis for :inaccessiblememonly, each statement is initially allowed to have the INACCESSIBLEMEM_OR_ARGMEMONLY effect, which may then be refined in adjust_effects if the arguments are proven to not have any mutability. This approach effectively replaces escape analysis with a simpler type-level analysis in the abstract interpretation phase for efficiency. While this refinement is more conservative than escape analysis, it should still be correct.

@gbaraldi
Copy link
Member Author

gbaraldi commented Dec 15, 2023

So the confusing thing, is that if we call a function with no args, that calls a function that is ?c and ?m we somehow refine it to c m. The thing that we're missing is that we never decay the ?c to !c, even after adjust_effects. And somehow the arguments of the caller then affect how we treat this.
The weird thing is that the inaccessiblemem_or_argmemonly isn't something that the caller should inherit from the callee, because they aren't the same arguments.

@vchuravy
Copy link
Sponsor Member

So the confusing thing, is that if we call a function with no args, that calls a function that is ?c and ?m we somehow refine it to c m. T

IIUC ?c correctly its m -> c e.g. if the calling function is proven to be inaccessiblememonly we are allowed to refine ?c to c.
It is not a statement over getfield itself, essentially we don't have dataflow analysis for the obj and so we say "as long as this function globally doesn't access argmem or global mem" we are allowed to refine.

KristofferC pushed a commit that referenced this pull request Dec 15, 2023
What observed in #52531 is that `QuoteNode` can embed global variables
that users can modify. Therefore, when dealing with `QuoteNode`, it's
necessary to taint its `:inaccessiblememonly` just like we do for
`GlobalRef`.

- fixes #52531
- replaces #52536
@giordano
Copy link
Contributor

Can this be closed now that #52548 has been merged?

@gbaraldi gbaraldi closed this Dec 15, 2023
@gbaraldi gbaraldi deleted the gb/conservative-mem-effect branch December 15, 2023 22:50
aviatesk added a commit that referenced this pull request Dec 16, 2023
What observed in #52531 is that `QuoteNode` can embed global variables
that users can modify. Therefore, when dealing with `QuoteNode`, it's
necessary to taint its `:inaccessiblememonly` just like we do for
`GlobalRef`.

- fixes #52531
- replaces #52536
@gbaraldi gbaraldi restored the gb/conservative-mem-effect branch January 10, 2024 17:55
@gbaraldi gbaraldi reopened this Jan 10, 2024
@gbaraldi gbaraldi closed this Jan 10, 2024
@giordano giordano deleted the gb/conservative-mem-effect branch January 10, 2024 18:30
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.

Mutable let block variable treated as immutable in Julia 1.10
4 participants