Skip to content

Conversation

@agle
Copy link
Contributor

@agle agle commented Sep 17, 2025

  • adds read-uninitialised locals invariant check: this is a correctness requirement of the parameter transform
  • for this to hold for all procedures param analysis needs interproc liveness results for every procedure, not just those reachable from main. Still need a better approach for this for all IDE solver calls than currently implemented
  • fix interproc liveness propagating live local vars across returns
  • make copyprop throw on subst cycle with useful error message

@agle agle force-pushed the read-uninit branch 2 times, most recently from c664cda to ef8ca6b Compare September 17, 2025 03:53
@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2025

scalafmt check passed.

last updated at 90ebef7 (logs).

@agle agle marked this pull request as ready for review September 18, 2025 02:27
@agle
Copy link
Contributor Author

agle commented Sep 18, 2025

In test IntervalDSATest/cntlm globals - local phase 4 more procedures collapse now:

      "hlist_subcmp_all",
      "headers_recv",
      "scanner_hook",
      "config_open"

Not sure which change causes this, it doesnt look like the inlining fix or the change to liveness alone cause it.

@agle
Copy link
Contributor Author

agle commented Sep 19, 2025

Seems fixed now, idk why I couldn't get the tail-recursive find op to work, probably a skill issue.

@l-kent
Copy link
Contributor

l-kent commented Sep 19, 2025

Is the read-uninitialised invariant check supposed to catch the sort of case we had with cntlm_noduk?

If I revert the fix for the inter-procedural live variables analysis and run the cntlm-no-duk InvervalDSATest, the infinite loop in the copy prop is identified and an exception is thrown, but the read-uninitialised invariant doesn't seem to catch any of that?

@l-kent
Copy link
Contributor

l-kent commented Sep 19, 2025

The read-uninitialised invariant does seem to fail for some of the cases in MemoryTransformTests, although it isn't causing the tests to fail

@agle
Copy link
Contributor Author

agle commented Sep 19, 2025

It detects it when placing the invariant check in the simplify pipeline before the copyprop pass, immediately after DSA, I'll commit a check there too.

@agle
Copy link
Contributor Author

agle commented Sep 19, 2025

The read-uninitialised invariant does seem to fail for some of the cases in MemoryTransformTests, although it isn't causing the tests to fail

In runutils after the memory transform I run the check but don't assert it passes as it seems to not conform; I expect the transform would also need to update parameter lists in some cases but I'm not sure the exact reasons it fails.

val locals = res.dsa.get.local
assert(locals.values.forall(_.glIntervals.size == 1))

println(locals.values.filterNot(IntervalDSA.checksStackMaintained).map(_.proc.procName).toSet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this just for debugging purposes?

Copy link
Member

@katrinafyi katrinafyi 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 and it has worked in tv.

is it worth a comment that read-uninit only checks in RPO order rather than dominance? because of this, it is possible for it to miss some uninitialised reads.

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.

4 participants