llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.63k stars 283 forks source link

[FIRRTL] Lexing Issues With Deeply Nested Expressions? #1771

Open seldridge opened 3 years ago

seldridge commented 3 years ago

Circuits with long lines and/or deeply nested expressions seem to crash the lexer.

Here's a failing circuit (this is a 1024-deep concatenation):

circuit Bar :
  module Bar :
    input a: UInt<1>
    output b: UInt<1024>

    b <= cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, a)))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))

This is erroring out with an EXC_BAD_ACCESS occurring in the lexer:

# lldb firtool -- Bar.fir
(lldb) target create "firtool"
Current executable set to 'firtool' (x86_64).
(lldb) settings set -- target.run-args  "Bar.fir"
(lldb) run
Process 10399 launched: '/Users/schuylere/usr/bin/firtool' (x86_64)
Process 10399 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x7ffeef3fffff)
    frame #0: 0x00000001005857e4 firtool`llvm::isAlpha(C=<unavailable>) at StringExtras.h:95
   92   inline bool isHexDigit(char C) { return hexDigitValue(C) != ~0U; }
   93   
   94   /// Checks if character \p C is a valid letter as classified by "C" locale.
-> 95   inline bool isAlpha(char C) {
   96     return ('a' <= C && C <= 'z') || ('A' <= C && C <= 'Z');
   97   }
   98   
Target 0: (firtool) stopped.
(lldb) frame select 1
frame #1: 0x000000010024a592 firtool`circt::firrtl::FIRLexer::lexIdentifierOrKeyword(this=0x00007ffeefbfdbe8, tokStart="cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, cat(a, a)))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))"...) at FIRLexer.cpp:386:10
   383  ///
   384  FIRToken FIRLexer::lexIdentifierOrKeyword(const char *tokStart) {
   385    // Match the rest of the identifier regex: [0-9a-zA-Z_$-]*
-> 386    while (llvm::isAlpha(*curPtr) || llvm::isDigit(*curPtr) || *curPtr == '_' ||
   387           *curPtr == '$' || *curPtr == '-')
   388      ++curPtr;
   389
lattner commented 3 years ago

Interesting, this is a novel problem to me, a consequence of using a recursive descent parser, but also using threads. The problem is that individual threads don't have a deep stack so we run into issues like this. We can make this incrementally better, but I'm not sure how to fix this.

lattner commented 3 years ago

Turning off multithreading probably "fixes" this.

darthscsi commented 3 years ago

At least on linux, all threads, including the main one have the same stack size. We can vary the stack size (ulimit -s) in linux, and that is picked up by pthreads, pushing this problem out.

parsePrimExp and parseExpImpl are the mutually recursive functions dominating the stack. parsePrimExp is using O(2k) bytes per frame. I trimmed it slightly in https://github.com/llvm/circt/commit/08c19bb7fcff2891093a0f4c40df377dde39aee2, but it still seems out-sized for the variables it has. I haven't checked if some poor behaving thing is being inlined. Debug builds have ~2.5x larger stackframes for this function.

lattner commented 3 years ago

At least on linux, all threads, including the main one have the same stack size.

Are you sure about that? Main is typically different - it's stack grows down from the top of the address space, whereas pthreads have to be individually allocated, and typically get a fixed size allocation. This is one of the challenges of getting standard pthreads to scale to 100K threads: you get too many ~2M stacks.

parsePrimExp and parseExpImpl are the mutually recursive functions dominating the stack. parsePrimExp is using O(2k) bytes per frame. I trimmed it slightly in 08c19bb, but it still seems out-sized for the variables it has.

Awesome, I agree - that does sound wacky

darthscsi commented 3 years ago

On Mon, Sep 13, 2021 at 2:38 PM Chris Lattner @.***> wrote:

At least on linux, all threads, including the main one have the same stack size.

Are you sure about that? Main is typically different - it's stack grows down from the top of the address space, whereas pthreads have to be individually allocated, and typically get a fixed size allocation. This is one of the challenges of getting standard pthreads to scale to 100K threads: you get too many ~2M stacks.

It depends on a few factors. pthreads will default to 2M stacks if there is no ulimit set, but use ulimit if it is set. On any Redhat and Debian derived system I've seen, the default ulimit is set to 8MB.

From pthread_create man page:

Under the NPTL threading implementation, if the RLIMIT_STACK soft resource limit at the time the program started has any value other than "unlimited", then it determines the default stack size of new threads.

darthscsi commented 3 years ago

The same RLIMIT_STACK will also control the size of the main stack at exec time, and I think prevent growth, so in common configuration, the main thread probably has less effective stack space due to environment.