oils-for-unix / oils

Oils is our upgrade path from bash to a better language and runtime. It's also for Python and JavaScript users who avoid shell!
http://www.oilshell.org/
Other
2.85k stars 159 forks source link

Crash with dict literal #2078

Open jtb20 opened 2 months ago

jtb20 commented 2 months ago

This testcase crashes ysh 0.23.0:

#!/usr/bin/env ysh
var DelegatedCompName = {
  "llvm"                 : "x_project",
  "rocprofiler_register" : "x_rocprofiler_register",
  "roct_thunk_interface" : "x_roct",
  "rocr_runtime"         : "x_rocr",
  "openmp"               : "x_openmp",
  "offload"              : "x_offload",
  "aomp_extras"          : "x_extras",
  "comgr"                : "x_comgr",
  "rocminfo"             : "x_rocminfo",
  "rocsmilib"            : "x_rocm_smi_lib",
  "amdsmi"               : "x_amdsmi",
  "flang_legacy"         : "x_flang_legacy",
  "pgmath"               : "x_pgmath",
  "flang"                : "x_flang",
  "flang_runtime"        : "x_flang_runtime",
  "hipcc"                : "x_hipcc",
  "hipamd"               : "x_hipamd",
  "rocm_dbgapi"          : "x_rocdbgapi",
  "rocgdb"               : "x_rocgdb",
  "roctracer"            : "x_roctracer",
  "rocprofiler"          : "x_rocprofiler"
}

It crashes here:

$ ysh crash.ysh
ysh: cpp/pgen2.cc:34: pnode::PNode* pnode::PNodeAllocator::NewPNode(int, syntax_asdl::Token*): Assertion `arena_->size() < arena_->capacity()' failed.
Aborted (core dumped)

I've not debugged further than that.

andychu commented 2 months ago

Thank you for finding this! It looks like we have an arbitrary limit here for some reason, I will fix it

andychu commented 2 months ago

I worked around this, in this commit:

http://op.oilshell.org/uuu/github-jobs/7900/

in case you want to get some work done ... But I guess there is also the workaround of using setvar on the dict


The underlying issue is having a large dict literal, and a hard-coded limit in our pgen2 parser generator

which is due to an interaction between the GC and std::vector pointer invalidation on resizing -- so we enforce that it doesn't resize, which means it must be fixed

I am thinking about which other data structure to switch to! to make it arbitrary size

jtb20 commented 2 months ago

Thank you for the quick response! I'll apply one or the other workaround for the time being.

jtb20 commented 2 months ago

(std::deque maybe?)

andychu commented 2 months ago

Oh yeah I didn't realize std::deque doesn't invalidate, so it's true it could be used

But I am just implementing a tiny subset of it, which is basically a linked list of pieces/chunks

Eventually I want to get rid of the libstdc++ dependency, like fish shell does, because we only use a few simple things (mainly std::vector), and the extra shared library isn't trivial

andychu commented 2 months ago

Hm that was a good suggestion, it just worked with a tiny change

https://op.oilshell.org/uuu/github-jobs/7901/

My own "simple" linked array arena didn't work, and also I realize I don't really know how to implement the equivalent of emplace_back() for it :-/


I will look at the performance and code size for std::deque and see if we want to keep it ... the source code is certainly short :)