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

Compiler crash in HeapToStack optimization #4059

Closed
redvers opened this issue Mar 16, 2022 · 12 comments · Fixed by #4067
Closed

Compiler crash in HeapToStack optimization #4059

redvers opened this issue Mar 16, 2022 · 12 comments · Fixed by #4067
Assignees

Comments

@redvers
Copy link
Contributor

redvers commented Mar 16, 2022

My offending code is in the following playground link: https://playground.ponylang.io/?gist=6e4fdaad3b062b9e8a1bffe457e5e422

It compiles when I call ponyc with the -d option. The compiler crashes (SEGV) when the -d is absent.

Here is the output from the compiler:

red@ub0:~/projects/pp$ ponyc -r=ir
Building builtin -> /home/red/.local/share/ponyup/ponyc-release-0.49.1-x86_64-linux-ubuntu20.04/packages/builtin
Building . -> /home/red/projects/pp
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
 Descriptors
Optimising
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Running pass 'Function Pass Manager' on module 'pp'.
1.      Running pass 'Move heap allocations to the stack' on function '@Main_Dispatch'
Segmentation fault (core dumped)

Here is the backtrace:

red@ub0:~/projects/pp$ lldb ponyc
(lldb) target create "ponyc"
Current executable set to 'ponyc' (x86_64).
(lldb) run
Process 23180 launched: '/home/red/.local/share/ponyup/bin/ponyc' (x86_64)
Building builtin -> /home/red/.local/share/ponyup/ponyc-release-0.49.1-x86_64-linux-ubuntu20.04/packages/builtin
Building . -> /home/red/projects/pp
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
 Descriptors
Optimising
Process 23180 stopped
* thread #1, name = 'ponyc', stop reason = signal SIGSEGV: invalid address (fault address: 0x28)
    frame #0: 0x0000000001eafee4 ponyc`llvm::CallGraphNode::removeCallEdgeFor(llvm::CallBase&) + 52
ponyc`llvm::CallGraphNode::removeCallEdgeFor:
->  0x1eafee4 <+52>: addl   $-0x1, 0x28(%rax)
    0x1eafee8 <+56>: movq   0x18(%r14), %rbp
    0x1eafeec <+60>: leaq   -0x28(%rbp), %rsi
    0x1eafef0 <+64>: movq   %rbx, %rdi
(lldb) bt
* thread #1, name = 'ponyc', stop reason = signal SIGSEGV: invalid address (fault address: 0x28)
  * frame #0: 0x0000000001eafee4 ponyc`llvm::CallGraphNode::removeCallEdgeFor(llvm::CallBase&) + 52
    frame #1: 0x00000000007119dc ponyc`HeapToStack::runOnInstruction(llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>&, llvm::Instruction*, llvm::DominatorTree&, llvm::Function&) + 1196
    frame #2: 0x0000000000711475 ponyc`HeapToStack::runOnFunction(llvm::Function&) + 389
    frame #3: 0x000000000087706d ponyc`llvm::FPPassManager::runOnFunction(llvm::Function&) + 1053
    frame #4: 0x000000000087cd93 ponyc`llvm::FPPassManager::runOnModule(llvm::Module&) + 51
    frame #5: 0x0000000000877670 ponyc`llvm::legacy::PassManagerImpl::run(llvm::Module&) + 928
    frame #6: 0x000000000070e0a1 ponyc`genopt + 1121
    frame #7: 0x00000000006f8d07 ponyc`genexe + 535
    frame #8: 0x00000000006f07cf ponyc`codegen + 143
    frame #9: 0x00000000006e602a ponyc`main + 634
    frame #10: 0x00007ffff7c2d0b3 libc.so.6`__libc_start_main + 243
    frame #11: 0x00000000006e5cee ponyc`_start + 46
(lldb)  

If there is any other information or help I can help provide, please let me know.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 16, 2022
@SeanTAllen
Copy link
Member

A more minimal example would help if possible.

@redvers
Copy link
Contributor Author

redvers commented Mar 16, 2022

Here is the most minimal example I can make:

actor Main
  new create(env: Env) =>
    let p: HtmlNode = P
    p.apply()

class P is HtmlNode
  var childlist: Array[HtmlNode] = []
  fun ref get_childlist(): Array[HtmlNode] => childlist

interface HtmlNode
  fun ref get_childlist(): Array[HtmlNode]
  fun ref apply(): None  =>
    for f in get_childlist().values() do
      None
    end

@SeanTAllen
Copy link
Member

Is the interface required or does it still crash if all the interface functionality is moved into the class?

@redvers
Copy link
Contributor Author

redvers commented Mar 16, 2022

Found the issue I think:

If you modify Line 3 to be let p: P = P then it will compile.

@redvers
Copy link
Contributor Author

redvers commented Mar 16, 2022

Is the interface required or does it still crash if all the interface functionality is moved into the class?

So, if I do that I have to leave at least a stub for apply() in the interface:

actor Main
  new create(env: Env) =>
    let p: HtmlNode = P
    p.apply()

class P is HtmlNode
  var childlist: Array[HtmlNode] val = []
  fun get_childlist(): Array[HtmlNode] val => childlist
  fun apply(): None  =>
    for f in get_childlist().values() do
      None
    end

interface HtmlNode
  fun apply(): None

It's almost like in my let p: HtmlNode = P like it's treating HtmlNode as a concrete type.

@SeanTAllen
Copy link
Member

My question was... does it crash without the interface or is the interface required. Leaving the interface doesn't answer that question. I'll check when I have time to see if the interface appears to be part of the issue.

@redvers
Copy link
Contributor Author

redvers commented Mar 16, 2022

When I remove the interface the type HtmlNode no longer exists which means my assignment isn't valid - so I have to remove all references to it.

actor Main
  new create(env: Env) =>
    let p: P = P
    p.apply()

class P
  var childlist: Array[P] val = []
  fun get_childlist(): Array[P] val => childlist
  fun apply(): None  =>
    for f in get_childlist().values() do
      None
    end

Even if I leave the interface in and everything else hooked up, changing the type of p is sufficient to make it compile cleanly:

actor Main
  new create(env: Env) =>
    let p: P = P
    p.apply()

class P is HtmlNode
  var childlist: Array[HtmlNode] val = []
  fun get_childlist(): Array[HtmlNode] val => childlist

interface HtmlNode
  fun get_childlist(): Array[HtmlNode] val
  fun apply(): None  =>
    for f in get_childlist().values() do
      None
    end

@SeanTAllen
Copy link
Member

Does the interface-less version crash? It sounds like no, but you haven't explicitly said that it doesn't.

@redvers
Copy link
Contributor Author

redvers commented Mar 16, 2022

It does not crash.

@SeanTAllen SeanTAllen changed the title Compiler crash in -r=ir phase Compiler crash when optimizing Mar 16, 2022
@SeanTAllen SeanTAllen added help wanted Extra attention is needed bug labels Mar 16, 2022
@SeanTAllen SeanTAllen changed the title Compiler crash when optimizing Compiler crash in HeapToStack optimization Mar 16, 2022
@SeanTAllen
Copy link
Member

SeanTAllen commented Mar 16, 2022

More minimal example:

actor Main
  new create(env: Env) =>
    let p: HtmlNode = P
    p.apply()

class P is HtmlNode
  fun apply(): None  =>
    for f in [as HtmlNode:].values() do
      None
    end

interface HtmlNode
  fun apply(): None

I feel like we either fixed a bug similar to this or still have an issue open for one that feels an awful lot like this.

@ergl
Copy link
Member

ergl commented Mar 17, 2022

@SeanTAllen It does remind me of #3784 a bit. See this comment: #3784 (comment)

It's not the same in the sense of having multiple messages, but in the fact that some optimization pass might be doing some unexpected things.

@SeanTAllen
Copy link
Member

There's a fix for this coming tomorrow.

@SeanTAllen SeanTAllen self-assigned this Mar 22, 2022
@SeanTAllen SeanTAllen removed help wanted Extra attention is needed discuss during sync Should be discussed during an upcoming sync labels Mar 22, 2022
SeanTAllen added a commit that referenced this issue Mar 23, 2022
This is a good one. One the March 22, 2022 development sync,
we collectively debugged issue #4059. We finally arrived at
the conclusion that our call graph usage was incorrect.

Originally during the call, Joe realized that while we were
using, we weren't doing it correctly. The initial assumption
was that the incorrect usage was innocous, however, it was
determined shortly thereafter that something about the incorrect
usage was in fact causing issue 4059.

This commit removes the offending code.

Gordon reported during the call that when the call graph code
was added as part the (now years ago) addition of LLVM 4 support
that the code was cribbed from the Dlang compiler and he posited,
probably not correctly.

Fixes #4059
SeanTAllen added a commit that referenced this issue Mar 23, 2022
This is a good one. One the March 22, 2022 development sync,
we collectively debugged issue #4059. We finally arrived at
the conclusion that our call graph usage was incorrect.

Originally during the call, Joe realized that while we were
using, we weren't doing it correctly. The initial assumption
was that the incorrect usage was innocous, however, it was
determined shortly thereafter that something about the incorrect
usage was in fact causing issue 4059.

This commit removes the offending code.

Gordon reported during the call that when the call graph code
was added as part the (now years ago) addition of LLVM 4 support
that the code was cribbed from the Dlang compiler and he posited,
probably not correctly.

Fixes #4059
SeanTAllen added a commit that referenced this issue Mar 23, 2022
This is a good one. One the March 22, 2022 development sync,
we collectively debugged issue #4059. We finally arrived at
the conclusion that our call graph usage was incorrect.

Originally during the call, Joe realized that while we were
using, we weren't doing it correctly. The initial assumption
was that the incorrect usage was innocous, however, it was
determined shortly thereafter that something about the incorrect
usage was in fact causing issue 4059.

This commit removes the offending code.

Gordon reported during the call that when the call graph code
was added as part the (now years ago) addition of LLVM 4 support
that the code was cribbed from the Dlang compiler and he posited,
probably not correctly.

Fixes #4059
SeanTAllen added a commit that referenced this issue Mar 23, 2022
This is a good one. One the March 22, 2022 development sync,
we collectively debugged issue #4059. We finally arrived at
the conclusion that our call graph usage was incorrect.

Originally during the call, Joe realized that while we were
using, we weren't doing it correctly. The initial assumption
was that the incorrect usage was innocous, however, it was
determined shortly thereafter that something about the incorrect
usage was in fact causing issue 4059.

This commit removes the offending code.

Gordon reported during the call that when the call graph code
was added as part the (now years ago) addition of LLVM 4 support
that the code was cribbed from the Dlang compiler and he posited,
probably not correctly.

Fixes #4059
@SeanTAllen SeanTAllen removed the bug label Jan 22, 2025
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 a pull request may close this issue.

4 participants