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

Phase Assembly is broken #8755

Open
scabug opened this Issue Jul 28, 2014 · 3 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Jul 28, 2014

The phase assembly, which is responsible for ensuring correct execution order of internal as well as user-defined compiler components, is severely broken.

How to reproduce

I have written a small compiler plugin which can be configured to instantiate an arbitrary number of noop components with user-defined phase constraints. For installing it locally, perform

$ git clone git@github.com:misberner/dummyphase.git
$ cd dummyphase
$ sbt publishLocal

The syntax for defining phases is: phase1(after;rightAfter;before)|phase2(after;rightAfter;before)|....
Here, after and before are comma-separated lists of phase names. The phase definition is specified as the property dummyphase.phases. Example invocations can be found below (note that the "parser" is extremely fragile).

If components specify no constraints, they are ignored

Every phase should run after "parser" and before "terminal". However, if a component specifies empty lists for both runsAfter and runsBefore, it is ignored entirely.

To make matters worse, a component is considered if it declares that it runs after parser, but not if it declares to run before terminal.

# A phase without any constraints, ignored
$ scalac -Xplugin ~/.ivy2/local/dummyphase/dummyphase_2.11/0.1-SNAPSHOT/jars/dummyphase_2.11.jar \
  -Ddummyphase.phases='phase(;;)' -Xshow-phases
# A phase that specifies it runs after the parser phase, shows up
$ scalac -Xplugin ~/.ivy2/local/dummyphase/dummyphase_2.11/0.1-SNAPSHOT/jars/dummyphase_2.11.jar \
  -Ddummyphase.phases='phase(parser;;)' -Xshow-phases
# A phase that specifies it runs before the terminal phase, ignored
$ scalac -Xplugin ~/.ivy2/local/dummyphase/dummyphase_2.11/0.1-SNAPSHOT/jars/dummyphase_2.11.jar \
  -Ddummyphase.phases='phase(;;terminal)' -Xshow-phases

If components specify runsRightAfter, all other constraints are ignored

This leads to wrong phase orders without any kind of warning.

# phase1 needs to run after delambdafy, phase2 needs to run *right after* phase1 and before icode.
# Since icode does not need to run *right* after delambdafy, a valid order would be:
# delambdafy, phase1, phase2, icode
$ scalac -Xplugin ~/.ivy2/local/dummyphase/dummyphase_2.11/0.1-SNAPSHOT/jars/dummyphase_2.11.jar \
  -Ddummyphase.phases='phase1(delambdafy;;)|phase2(;phase1;icode)' \
  -Xshow-phases
...
    delambdafy  22  remove lambdas
         icode  23  generate portable intermediate code
        phase1  24
        phase2  25
...

Solvable constraint systems are rejected

The current phase assembly fails if a phase that defines a rightAfter dependency has another outgoing edge. Due to the above, this cannot be shown by specifying an after and a rightAfter dependency, but a rightAfter in combination with a before exposes this.

$ scalac -Xplugin ~/.ivy2/local/dummyphase/dummyphase_2.11/0.1-SNAPSHOT/jars/dummyphase_2.11.jar \
  -Ddummyphase.phases='phase1(;patmat;)|phase2(parser;;phase1)' \
  -Xshow-phases
error: scala.reflect.internal.FatalError: Phase phase1 can't follow patmat, created phase-order.dot

As can be seen in the attached phase-order.png, the constraint system is satisfiable.

The implementation has bugs

The implementation modifies variable members of case classes (that are used for hashCode calculation), without removing them from containing HashSet s. This leads to erroneous phase graphs. See attached file phase-4.png, where the posterasure node is still present even though it was collapsed into another node.

Note that no user-defined phases are necessary to expose this behavior.

$ scalac -Xshow-phases -Xgenerate-phase-graph phase
$ dot -Tpng phase-4.dot >phase-4.png
@scabug

This comment has been minimized.

Copy link

scabug commented Jul 28, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8755?orig=1
Reporter: Malte Isberner (misberner)
Affected Versions: 2.11.1
Attachments:

  • phase-4.png (created on Jul 28, 2014 10:13:28 AM UTC, 154673 bytes)
  • phase-order.png (created on Jul 28, 2014 10:13:28 AM UTC, 183865 bytes)
@scabug

This comment has been minimized.

Copy link

scabug commented Jul 31, 2014

@som-snytt said:
Scaladoc is sample customizer. It had phase issues in 2.10, see comment on #7905.

@scabug

This comment has been minimized.

Copy link

scabug commented Aug 12, 2014

@paulp said:
The phase assembly code is some of the worst in the compiler. Rarely is a molehill of a problem given such mountainous treatment.

@scabug scabug added the critical label Apr 7, 2017

@SethTisue SethTisue removed the critical label Jul 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment