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

mirpasses: make call-argument fixup a MIR pass #818

Merged
merged 4 commits into from
Jul 31, 2023

Commits on Jul 29, 2023

  1. mirpasses: make call-argument fixup a MIR pass

    Summary
    =======
    
    Replace the `PNode`-based fixup for call arguments in `ccgcalls` with a
    MIR pass. This removes a dependency on `PNode`-based analysis from
    `cgen`, and also makes it possible to, in the future, enable the fixup
    when the JS or VM backends are used (both are also affected by the
    issue).
    
    While the used analysis stays mostly the same, an observable evaluation-
    order violation is fixed. Injecting (shallow) copies for values passed
    to both immutable and mutable parameters now considers *all* parameters,
    instead of only parameters to the right of immutable ones. For example,
    given:
    ```nim
    f(a, a) # proc f(x: var T, y: T)
    ```
    this fixes mutations through `x` inside `f` being visible on `y`.
    
    Details
    =======
    
    The analysis used by the MIR pass works mostly the same as the one
    previously used in `ccgcalls`: for each argument value that is not
    explicitly passed by-reference, it is analyzed whether:
    - the value is potentially mutated *after* it is bound to the parameter
      but *before* the procedure is called
    - the value is potentially also passed to a `var` parameter
    If either is the case, the argument is shallow-copied to a temporary
    that is then passed to the parameter instead.
    
    The differences compared to the previous analysis are that:
    - checking whether the argument value (or something that potentially
      overlaps with it in memory) is also passed to a mutable parameter
      now also considers preceding parameters. Previously, only the
      following parameters were checked
    - testing for overlapping values considers the whole path now, instead
      of only the root. In effect, this means that for `f(a.x, a.y)`, where
      the second parameter is mutable, no temporary is (unnecessarily)
      injected for the first parameter
    
    For overlap testing, the `maybeSameMutableLocation` procedure is
    introduced, which mirrors the behaviour of `dfa.aliases` (the routine
    used by the previous `PNode`-based analysis). Since dereferences of
    pointer-like values are treated like a normal field access, calls like
    `f(a[].x, b[].y)`, where one of the parameters is mutable and `a` and
    `b` point to the same location, still cause observable evaluation-order
    violations.
    
    Finally, the analysis and temporary injection in `ccgcalls` is removed,
    and a test is added for the fixed issue.
    zerbina committed Jul 29, 2023
    Configuration menu
    Copy the full SHA
    f791019 View commit details
    Browse the repository at this point in the history

Commits on Jul 30, 2023

  1. mirpasses: fix two critical bugs

    One bug was that the wrong nodes were compared (`^1` was used instead of
    `^i`), and while this doesn't necessarily lead to incorrect behaviour,
    it does lead to overlap being detected where there is none.
    
    The second was with the node position for array element comparisons
    being wrong.
    zerbina committed Jul 30, 2023
    Configuration menu
    Copy the full SHA
    b7822e5 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    487d553 View commit details
    Browse the repository at this point in the history
  3. refactor: move loops into a common block

    Co-authored-by: Clyybber <darkmine956@gmail.com>
    zerbina and Clyybber authored Jul 30, 2023
    Configuration menu
    Copy the full SHA
    7a97752 View commit details
    Browse the repository at this point in the history