Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Followup-For: Bug #1030284 I decline to participate further with this bugreport, although others are welcome to pick up from the patches I've submitted (please don't merge them as-is; modify them to apply corrections).
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >AssertionError [ERR_ASSERTION]: The input did not match the regular > expression /RangeError: Maximum call stack size exceeded/. Input: > >'Segmentation fault\n' You removed the subtraction of V8_STACK_RESERVE when mutilating my patch; this may be related (if it still appears with, raise the constant). You also removed the integer safety check (the comparison with 2³¹-1). Don’t. bye, //mirabilos -- (gnutls can also be used, but if you are compiling lynx for your own use, there is no reason to consider using that package) -- Thomas E. Dickey on the Lynx mailing list, about OpenSSL
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Followup-For: Bug #1030284 X-Debbugs-Cc: t...@mirbsd.de It does seem to (continue to) function, at least on x86: ~/dygraphs-2.2.0$ NODE_PATH=/usr/share/nodejs ../nodejs-18.13.0+dfsg1/out/Release/node /usr/bin/babeljs --config-file $PWD/babel.config.json --compact false --source-maps inline -d tests5 auto_tests Successfully compiled 59 files with Babel (2744ms). (NOTE: this was an x86 system, not an ARM64 system) And the relevant v8 help text output appears to update accordingly: ~/dygraphs-2.2.0$ ../nodejs-18.13.0+dfsg1/out/Release/node --v8-options | grep -i stack-size --stack-size (default size of stack region v8 is allowed to use (in kBytes)) type: int default: --stack-size=8192
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Followup-For: Bug #1030284
X-Debbugs-Cc: t...@mirbsd.de
Hmm.. although the build itself succeeded, there was a unit test failure that
appears related to the change:
not ok 3213 sequential/test-fs-stat-sync-overflow
---
duration_ms: 1.111
severity: fail
exitcode: 1
stack: |-
node:assert:1027
throw err;
^
AssertionError [ERR_ASSERTION]: The input did not match the regular
expression /RangeError: Maximum call stack size exceeded/. Input:
'Segmentation fault\n'
at
/root/nodejs-18.13.0+dfsg1/test/sequential/test-fs-stat-sync-overflow.js:40:10
at ChildProcess.exithandler (node:child_process:427:5)
at ChildProcess.emit (node:events:513:28)
at maybeClose (node:internal/child_process:1091:16)
at Socket. (node:internal/child_process:449:11)
at Socket.emit (node:events:513:28)
at Pipe. (node:net:321:12) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: 'Segmentation fault\n',
expected: /RangeError: Maximum call stack size exceeded/,
operator: 'match'
}
Node.js v18.13.0
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Followup-For: Bug #1030284
X-Debbugs-Cc: t...@mirbsd.de
Thanks, Thorsten. I'm currently rebuilding (on x86) from the attached patch,
adapted from yours.
* prefers a one-time repeat division over the clever-but-fragile div-assign
* removes the upperbound check because integer greater-than checks can be
problematic
* places comparison constants on the lhs for safety
I'll post test results when they are available.
Cheers,
James
Description: Request an rlimit-determined stack size from V8
Author: James Addison
Bug-Debian: https://bugs.debian.org/1030284
--- /dev/null
+++ nodejs-18.13.0+dfsg1/foo.c
@@ -0,0 +1,7 @@
+#include
+
+int main() {
+if (4 > (int)(8 / 3)) {
+printf("Hello world!");
+}
+}
--- nodejs-18.13.0+dfsg1.orig/src/node.cc
+++ nodejs-18.13.0+dfsg1/src/node.cc
@@ -785,6 +785,22 @@ int InitializeNodeWithArgs(std::vector (int)(lim.rlim_cur / 1024))
+ fprintf(stderr, "W: stack size adjustment: RLIMIT_STACK is too
small\n");
+ else
+ V8::SetFlagsFromString(stackSize, snprintf(stackSize,
sizeof(stackSize),
+ "--stack-size=%d", (int)(lim.rlim_cur / 1024)));
+}
+
HandleEnvOptions(per_process::cli_options->per_isolate->per_env);
#if !defined(NODE_WITHOUT_NODE_OPTIONS)
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit:
>... to add something like this:
Ouch, by going via a string?! I wouldn’t have thought of that…
> if (!(flags & ProcessInitializationFlags::kNoAdjustResourceLimits)) {
>struct rlimit lim;
>if (getrlimit(RLIMIT_STACK, ) == 0) {
> char stackSize[32];
32 is magic and may also be wrong here.
> int buflen = snprintf(stackSize, sizeof(stackSize),
>"--stack-size=%d", lim.rlim_cur);
%d isn’t right for lim.rlim_cur, which is of type rlim_t.
--stack-size seems to take in KiB, and we’d want a reserve.
> if (buflen < sizeof(stackSize)) {
>V8::SetFlagsFromString(stackSize, buflen);
> }
>}
> }
So, taking your next post into account, probably something more
like this:
#define V8_STACK_RESERVE 128
if (!(flags & ProcessInitializationFlags::kNoAdjustResourceLimits)) {
struct rlimit lim;
char stackSize[sizeof("--stack-size=") + /* 2³¹ */ 10];
if (getrlimit(RLIMIT_STACK, ))
fprintf(stderr, "W: stack size adjustment: cannot get
RLIMIT_STACK\n");
else if (lim.rlim_cur == RLIM_INFINITY)
fprintf(stderr, "W: stack size adjustment: RLIMIT_STACK is
unlimited\n");
else if ((lim.rlim_cur /= 1024) <= V8_STACK_RESERVE)
fprintf(stderr, "W: stack size adjustment: RLIMIT_STACK is too
small\n");
else if ((lim.rlim_cur -= V8_STACK_RESERVE) >= /* 2³¹ */ 2147483647)
fprintf(stderr, "W: stack size adjustment: RLIMIT_STACK is too
large\n");
else
V8::SetFlagsFromString(stackSize, snprintf(stackSize,
sizeof(stackSize),
"--stack-size=%d", (int)lim.rlim_cur));
}
Untested, still not back to full health, take with grains of salt.
bye,
//mirabilos
--
In traditional syntax ' is ignored, but in c99 everything between two ' is
handled as character constant. Therefore you cannot use ' in a preproces-
sing file in c99 mode. -- Ragge
No faith left in ISO C99, undefined behaviour, etc.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Sat, 13 May 2023 at 12:15, James Addison wrote:
>
> On Sat, 13 May 2023 at 11:14, James Addison wrote:
> >
> > On Sat, 13 May 2023 at 02:18, Thorsten Glaser wrote:
> > >
> > > James Addison dixit:
> > >
> > > >I'm going to stay involved with this thread, but I think that it is
> > > >upon you to develop or provide further guidance towards a patch if
> > > >it's something you'd like to have implemented, Thorsten.
> > >
> > > I actually have looked into that but I don’t understand the nodejs
> > > and v8 source code enough to be able. I know C, but not CFrustFrust.
> > > I would rather prefer asm…
> >
> > Ok, thanks. We may be stalled temporarily in that case.
> >
> > On Sat, 13 May 2023 at 00:20, James Addison wrote:
> > > That said: perhaps it could be useful if someone could check whether
> > > the following commit is relevant to this:
> > > https://github.com/libuv/libuv/commit/18c7530a75d813801f819caae4dff47fc4a1d4a1
> >
> > I ran the repro case (with some simplifications) from the GitHub
> > thread using 'strace' and grepped for rlimit-related syscalls:
> >
> > # on arm64, this currently replicates the problem using debian bookworm
> > $ strace babeljs --config-file $PWD/babel.config.json --compact
> > false --source-maps inline -d tests5 auto_tests 2>&1 | grep -i rlimit
> >
> > All of the resulting calls (on an ARM64 host) are to the 'prlimit64'
> > syscall and have a zero exit-code (success).
>
> How about extending the code around after this block:
> https://github.com/nodejs/node/blob/2bb4b59fa5529569ad38d3bf7d3c926d8e47/src/node.cc#L781-L786
>
> ... to add something like this:
>
> if (!(flags & ProcessInitializationFlags::kNoAdjustResourceLimits)) {
> struct rlimit lim;
> if (getrlimit(RLIMIT_STACK, ) == 0) {
> char stackSize[32];
> int buflen = snprintf(stackSize, sizeof(stackSize),
> "--stack-size=%d", lim.rlim_cur);
> if (buflen < sizeof(stackSize)) {
> V8::SetFlagsFromString(stackSize, buflen);
> }
> }
> }
>
> ?
Note: probably worth adjusting that to add another conditional to
check that the stack limit isn't unset/infinity:
if (getrlimit(RLIMIT_STACK, ) == 0 && lim.rlim_max != RLIM_INFINITY) {
I'm not sure that I have an ARM64 machine with enough memory and/or
non-flash-based disk to want to compile and test this on. But
hopefully it would be fairly straightforward (apt source nodejs, apt
build-dep ., dpkg-buildpackage, or something along those lines).
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Sat, 13 May 2023 at 11:14, James Addison wrote:
>
> On Sat, 13 May 2023 at 02:18, Thorsten Glaser wrote:
> >
> > James Addison dixit:
> >
> > >I'm going to stay involved with this thread, but I think that it is
> > >upon you to develop or provide further guidance towards a patch if
> > >it's something you'd like to have implemented, Thorsten.
> >
> > I actually have looked into that but I don’t understand the nodejs
> > and v8 source code enough to be able. I know C, but not CFrustFrust.
> > I would rather prefer asm…
>
> Ok, thanks. We may be stalled temporarily in that case.
>
> On Sat, 13 May 2023 at 00:20, James Addison wrote:
> > That said: perhaps it could be useful if someone could check whether
> > the following commit is relevant to this:
> > https://github.com/libuv/libuv/commit/18c7530a75d813801f819caae4dff47fc4a1d4a1
>
> I ran the repro case (with some simplifications) from the GitHub
> thread using 'strace' and grepped for rlimit-related syscalls:
>
> # on arm64, this currently replicates the problem using debian bookworm
> $ strace babeljs --config-file $PWD/babel.config.json --compact
> false --source-maps inline -d tests5 auto_tests 2>&1 | grep -i rlimit
>
> All of the resulting calls (on an ARM64 host) are to the 'prlimit64'
> syscall and have a zero exit-code (success).
How about extending the code around after this block:
https://github.com/nodejs/node/blob/2bb4b59fa5529569ad38d3bf7d3c926d8e47/src/node.cc#L781-L786
... to add something like this:
if (!(flags & ProcessInitializationFlags::kNoAdjustResourceLimits)) {
struct rlimit lim;
if (getrlimit(RLIMIT_STACK, ) == 0) {
char stackSize[32];
int buflen = snprintf(stackSize, sizeof(stackSize),
"--stack-size=%d", lim.rlim_cur);
if (buflen < sizeof(stackSize)) {
V8::SetFlagsFromString(stackSize, buflen);
}
}
}
?
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Sat, 13 May 2023 at 02:18, Thorsten Glaser wrote: > > James Addison dixit: > > >I'm going to stay involved with this thread, but I think that it is > >upon you to develop or provide further guidance towards a patch if > >it's something you'd like to have implemented, Thorsten. > > I actually have looked into that but I don’t understand the nodejs > and v8 source code enough to be able. I know C, but not CFrustFrust. > I would rather prefer asm… Ok, thanks. We may be stalled temporarily in that case. On Sat, 13 May 2023 at 00:20, James Addison wrote: > That said: perhaps it could be useful if someone could check whether > the following commit is relevant to this: > https://github.com/libuv/libuv/commit/18c7530a75d813801f819caae4dff47fc4a1d4a1 I ran the repro case (with some simplifications) from the GitHub thread using 'strace' and grepped for rlimit-related syscalls: # on arm64, this currently replicates the problem using debian bookworm $ strace babeljs --config-file $PWD/babel.config.json --compact false --source-maps inline -d tests5 auto_tests 2>&1 | grep -i rlimit All of the resulting calls (on an ARM64 host) are to the 'prlimit64' syscall and have a zero exit-code (success).
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >I'm going to stay involved with this thread, but I think that it is >upon you to develop or provide further guidance towards a patch if >it's something you'd like to have implemented, Thorsten. I actually have looked into that but I don’t understand the nodejs and v8 source code enough to be able. I know C, but not CFrustFrust. I would rather prefer asm… bye, //mirabilos -- When he found out that the m68k port was in a pretty bad shape, he did not, like many before him, shrug and move on; instead, he took it upon himself to start compiling things, just so he could compile his shell. How's that for dedication. -- Wouter, about my Debian/m68k revival
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Fri, 12 May 2023 at 23:23, James Addison wrote: > > On Fri, 12 May 2023 at 16:54, Thorsten Glaser wrote: > > > > Yes, but given the usual ulimit, the new limit would be 4+ times > > the old one, much much harder to reach. > > That does sound promising. > > I've followed up on this discussion with the relevant upstream NodeJS > thread, and beyond there to the relevant V8 discussion group. My > sense from those, and given my own experience building NodeJS is that > I don't feel an rlimit patch is straightforward or worthwhile - > although it's possible that I didn't accurately understand or > communicate the context. > > I'm going to stay involved with this thread, but I think that it is > upon you to develop or provide further guidance towards a patch if > it's something you'd like to have implemented, Thorsten. Maybe my tone was unclear, but I'm not hugely keen to provide more effort on this -- despite being interested -- because I feel like I've been running errands to try to find a good path through, when in fact I don't really understand the nature of the problem, nor am I likely to benefit much from it. But if improvement is possible, I'll do what I can. That said: perhaps it could be useful if someone could check whether the following commit is relevant to this: https://github.com/libuv/libuv/commit/18c7530a75d813801f819caae4dff47fc4a1d4a1
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Fri, 12 May 2023 at 16:54, Thorsten Glaser wrote: > > Yes, but given the usual ulimit, the new limit would be 4+ times > the old one, much much harder to reach. That does sound promising. I've followed up on this discussion with the relevant upstream NodeJS thread, and beyond there to the relevant V8 discussion group. My sense from those, and given my own experience building NodeJS is that I don't feel an rlimit patch is straightforward or worthwhile - although it's possible that I didn't accurately understand or communicate the context. I'm going to stay involved with this thread, but I think that it is upon you to develop or provide further guidance towards a patch if it's something you'd like to have implemented, Thorsten.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >So: a fix here won't achieve stack capacity equality across No. The fix you proposed won’t achieve that but others would improve the situation much more, so that equality across arches won’t need to matter any more. >Or, to put it another way: applying an increase (either static or >dynamic, either ARM-specific or across all architectures) for stack >size determination would move the problem, and another architecture >would take the place of "architecture where RangeError can occur in >code x that doesn't occur on other architectures". Yes, but given the usual ulimit, the new limit would be 4+ times the old one, much much harder to reach. >it, though - and based on their current policy, NodeJS upstream seem >unlikely to accept it since they don't want to modify their vendored >V8. AIUI that’s not necessary because you can already set the stack limit with a nodejs command line option. The patch could just set the limit, using the same facility that CLI option uses, if that option isn’t given (or before it is processed). bye, //mirabilos -- (gnutls can also be used, but if you are compiling lynx for your own use, there is no reason to consider using that package) -- Thomas E. Dickey on the Lynx mailing list, about OpenSSL
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Thu, 11 May 2023 at 23:54, Thorsten Glaser wrote: > > James Addison dixit: > > >On Thu, 11 May 2023 at 02:43, Andres Salomon wrote: > > >> For ARM64, he says that raising the stack limit is not safe for v8 > >> *embedded inside WebView*, and therefore not appropriate for upstream > >> v8. But then he says it could/should be safe for v8 *embedded inside > >> NodeJS*. > >> > >> Based on that, I suggest patching Debian's NodeJS with the patch to > >> adjust armhf/arm64 stack limit size > > That would be a good thing (huh, wasn’t armhf good?), but… > > >I have a question: if we apply the patch and begin using the same > >constant stack size of 984kb on 32-bit ARM and 64-bit ARM as is > >defined for other architectures, then does NodeJS on those platforms > >begin supporting exactly the same stack frame capacity (maximum call > >depth for any given recursive function, for example) as a build of the > >same NodeJS source on x86 and amd64 respectively? > > … no, because both stack usage and other stuff on stack differ. Ok, that's what I thought, but I'm not familiar with the details here. So: a fix here won't achieve stack capacity equality across architectures. (I say this because I think we should be clear about what the bugreport is about, and, where possible, the known limitations of fixes) Or, to put it another way: applying an increase (either static or dynamic, either ARM-specific or across all architectures) for stack size determination would move the problem, and another architecture would take the place of "architecture where RangeError can occur in code x that doesn't occur on other architectures". Do those statements seem true? (they make sense to me, but I also think it's possible that I've misunderstood something here) > Which is why I’d rather have the getrlimit-based one for nodejs. > That would give us twice to four times the limit. That makes sense, and I agree that dynamic stack-sizing could help (perhaps quite a lot on some systems). We'd need a patch to implement it, though - and based on their current policy, NodeJS upstream seem unlikely to accept it since they don't want to modify their vendored V8. But if it showed significant benefits then perhaps we could use that to contribute to further discussion with either/both of those projects.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >On Thu, 11 May 2023 at 02:43, Andres Salomon wrote: >> For ARM64, he says that raising the stack limit is not safe for v8 >> *embedded inside WebView*, and therefore not appropriate for upstream >> v8. But then he says it could/should be safe for v8 *embedded inside >> NodeJS*. >> >> Based on that, I suggest patching Debian's NodeJS with the patch to >> adjust armhf/arm64 stack limit size That would be a good thing (huh, wasn’t armhf good?), but… >I have a question: if we apply the patch and begin using the same >constant stack size of 984kb on 32-bit ARM and 64-bit ARM as is >defined for other architectures, then does NodeJS on those platforms >begin supporting exactly the same stack frame capacity (maximum call >depth for any given recursive function, for example) as a build of the >same NodeJS source on x86 and amd64 respectively? … no, because both stack usage and other stuff on stack differ. Which is why I’d rather have the getrlimit-based one for nodejs. That would give us twice to four times the limit. >> (As chromium maintainer, which also embeds v8, I haven't heard of any >> issues and hadn't planned on touching stacks limits. It sure would be Yes, yes, definitely don’t change it outside of nodejs. >> javascript code that is triggering this bug should really be fixed to >> not be so stack-intensive, of course. Perhaps this bug cloned at a >> lower severity, filed against those packages that this bug is affecting? That’s got a dependency chain so long that don’t hold your breath for ALL those to be changed is true. Besides, how would the respective maintainers be aware of this in the first place? Short of failing with an arbitrary nesting limit on all arches, in upstream nodejs, I doubt you’d get half of the upstream code maintainers to care… if they even care about their code any more at all. I am not a friend of such limits either, in addition. bye, //mirabilos -- Solange man keine schmutzigen Tricks macht, und ich meine *wirklich* schmutzige Tricks, wie bei einer doppelt verketteten Liste beide Pointer XORen und in nur einem Word speichern, funktioniert Boehm ganz hervorragend. -- Andreas Bogk über boehm-gc in d.a.s.r
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Thu, 11 May 2023 at 02:43, Andres Salomon wrote: > > On Sat, 11 Mar 2023 11:04:15 + James Addison > wrote: > > Package: nodejs > > Followup-For: Bug #1030284 > > X-Debbugs-Cc: t...@debian.org > > > > Guidance received from the V8 project (a vendored dependency in the > upstream > > NodeJS codebase) on the v8-dev mailing list is, in > summary/interpretation: > > > > * It is not yet safe to increase the stack size limit on ARM64 > systems. > [...] > > Sidenotes: > > > > A patch for 32-bit architectures could apparently be acceptable, > although may > > be best offered to NodeJS rather than V8. For what it's worth: > NodeJS seems > > to have a policy of not accepting patches to their vendored > dependencies. > > > > In reading Jakob's response > (https://groups.google.com/g/v8-dev/c/7ZI3vxtabcU/m/c9qvHkOBBAAJ), I'm > interpreting it slightly differently- > > He says that raising the stack limit *is* safe for 32-bit ARM, even > inside of the V8 upstream source tree. > > For ARM64, he says that raising the stack limit is not safe for v8 > *embedded inside WebView*, and therefore not appropriate for upstream > v8. But then he says it could/should be safe for v8 *embedded inside > NodeJS*. > > Based on that, I suggest patching Debian's NodeJS with the patch to > adjust armhf/arm64 stack limit size to 984kb. With the caveat that the > javascript code that is triggering this bug should really be fixed to > not be so stack-intensive, of course. Perhaps this bug cloned at a > lower severity, filed against those packages that this bug is affecting? > > (As chromium maintainer, which also embeds v8, I haven't heard of any > issues and hadn't planned on touching stacks limits. It sure would be > nice to have just one copy of V8 in the archive, though..) I have a question: if we apply the patch and begin using the same constant stack size of 984kb on 32-bit ARM and 64-bit ARM as is defined for other architectures, then does NodeJS on those platforms begin supporting exactly the same stack frame capacity (maximum call depth for any given recursive function, for example) as a build of the same NodeJS source on x86 and amd64 respectively?
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Sat, 11 Mar 2023 11:04:15 + James Addison wrote: > Package: nodejs > Followup-For: Bug #1030284 > X-Debbugs-Cc: t...@debian.org > > Guidance received from the V8 project (a vendored dependency in the upstream > NodeJS codebase) on the v8-dev mailing list is, in summary/interpretation: > > * It is not yet safe to increase the stack size limit on ARM64 systems. [...] > Sidenotes: > > A patch for 32-bit architectures could apparently be acceptable, although may > be best offered to NodeJS rather than V8. For what it's worth: NodeJS seems > to have a policy of not accepting patches to their vendored dependencies. > In reading Jakob's response (https://groups.google.com/g/v8-dev/c/7ZI3vxtabcU/m/c9qvHkOBBAAJ), I'm interpreting it slightly differently- He says that raising the stack limit *is* safe for 32-bit ARM, even inside of the V8 upstream source tree. For ARM64, he says that raising the stack limit is not safe for v8 *embedded inside WebView*, and therefore not appropriate for upstream v8. But then he says it could/should be safe for v8 *embedded inside NodeJS*. Based on that, I suggest patching Debian's NodeJS with the patch to adjust armhf/arm64 stack limit size to 984kb. With the caveat that the javascript code that is triggering this bug should really be fixed to not be so stack-intensive, of course. Perhaps this bug cloned at a lower severity, filed against those packages that this bug is affecting? (As chromium maintainer, which also embeds v8, I haven't heard of any issues and hadn't planned on touching stacks limits. It sure would be nice to have just one copy of V8 in the archive, though..)
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >Based on what I've learned about this bug, I believe that architecture-specific >behaviour related to stack sizes is inherent in the V8 library vendored by >upstream NodeJS. Yes, but the v8 library’s defaults are targetting a browser, and one whose uses are much wider, e.g. inside Android, than NodeJS’s itself. Which is why I believe that NodeJS very much can and certainly should fix this by setting suitable limits. >long time. Individual codebases such as the affected 'src:dygraphs' package >can also be improved to reduce the likelihood of call stack size overflow. It’s babel7 actually which runs into this. And I believe that, rather to the contrary, further evolution of software-written-in-NodeJS will let this error show up *more* and probably on nōn-arm64 as well(!), so I’d rather have NodeJS set proper limits instead of sticking to v8’s given the latter has a different scope and run-time environment than the former. >Given those, and the absence of any sense that this is a regression, I think we >should lower the priority of this bug to below release-critical. I very much disagree, this breaks arch:all packages on some platforms so it causes issues packagers who don’t themselves use arm64 cannot discover in the first place. We need to at least achieve architecture partity in Debian, pending better upstream fixes. bye, //mirabilos -- 16:47⎜«mika:#grml» .oO(mira ist einfach gut) 23:22⎜«mikap:#grml» mirabilos: und dein bootloader ist geil :)23:29⎜«mikap:#grml» und ich finds saugeil dass ich ein bsd zum booten mit grml hab, das muss ich dann gleich mal auf usb-stick installieren -- Michael Prokop über MirOS bsd4grml
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 X-Debbugs-Cc: t...@mirbsd.de On Thu, 02 Feb 2023 01:56:10 +, Thorsten wrote: > I consider this an architecture-specific release-critical bug. Maybe > having a reproducer and access to a porterbox will allow a nodejs > maintainer to track this down. Based on what I've learned about this bug, I believe that architecture-specific behaviour related to stack sizes is inherent in the V8 library vendored by upstream NodeJS. Improvements may be possible and we can continue to track and assist towards those, however there are likely to be runtime differences that remain for a long time. Individual codebases such as the affected 'src:dygraphs' package can also be improved to reduce the likelihood of call stack size overflow. Given those, and the absence of any sense that this is a regression, I think we should lower the priority of this bug to below release-critical.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 X-Debbugs-Cc: t...@debian.org Guidance received from the V8 project (a vendored dependency in the upstream NodeJS codebase) on the v8-dev mailing list is, in summary/interpretation: * It is not yet safe to increase the stack size limit on ARM64 systems. * For a given constant stack size, recursion depth is architecture-dependent, and so the patch to restore the 984K stack size on ARM64 would not provide equal recursion depth on all systems. * Exceeding stack depth limits is generally a sign of an application that would benefit from relevant refactoring (personal note: for example, by reducing the depth of recursion required, or by replacing a recursive algorithm with an equivalent iterative algorithm). Sidenotes: A patch for 32-bit architectures could apparently be acceptable, although may be best offered to NodeJS rather than V8. For what it's worth: NodeJS seems to have a policy of not accepting patches to their vendored dependencies. None of this rules out an rlimit-based approach as suggested by Thorsten.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 Echoing some notes and findings from the GitHub issue thread: * https://crbug.com/405338 seems inaccessible to at least two people * https://codereview.chromium.org/555943003 was initially proposed to resolve that bug * https://codereview.chromium.org/583163002 was chosen instead, for reasons of simplicity and performance concerns It is the latter codereview (583163002) that introduced the reduction in the stack size on ARM (that the patch in this bug thread removes). I'm planning to write to the V8 contributors mailing list to summarize these findings and ask whether they have any guidance/recommendations.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Followup-For: Bug #1030284 There is one part of this patch that worries me, and it is the line: > -//See issue crbug.com/405338 I'm not able to access that bug: neither anonymously nor when logged into my gmail account. The comment would seem to indicate that it contains relevant context about why the stack limit was reduced on ARM architectures.
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Hi, On Wed, Mar 01, 2023 at 02:41:05PM +0100, Jérémy Lal wrote: > For now I'm unlucky with the porterbox, because /var/run/schroot > disappeared yesterday. I can confirm that the issue isn't reproducible with V8_DEFAULT_STACK_SIZE_KB set to 984. Built and tested on a Macbook M1. (sid-arm64)ema@sarzana:~/debian/dygraphs-2.2.1 $ babeljs --config-file $PWD/babel.config.json --compact false --source-maps inline -d tests5 auto_tests Successfully compiled 59 files with Babel (994ms).
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Le mer. 1 mars 2023 à 14:39, James Addison a écrit : > If reproducible: would this bug be a good candidate for upload of a > fix to 'experimental' so that it can be alpha-tested by others? > Sure. For now I'm unlucky with the porterbox, because /var/run/schroot disappeared yesterday. Notified debian-admin. Jérémy
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
If reproducible: would this bug be a good candidate for upload of a fix to 'experimental' so that it can be alpha-tested by others? On Wed, 1 Mar 2023 at 02:55, Jérémy Lal wrote: > > > > Le mer. 1 mars 2023 à 02:30, Thorsten Glaser a écrit : >> >> Jérémy Lal dixit: >> >> >I can build nodejs on amhdal.debian.org if you're not comfortable with that. >> >> The problem with the DSA porterboxen is that you cannot install your own >> built packages in the chroot to use them there… unless there’s a >> solution not yet known to me? > > > Indeed, but the binary can be run from build dir, so I just need to try and > reproduce the bug from there. >
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Le mer. 1 mars 2023 à 02:30, Thorsten Glaser a écrit : > Jérémy Lal dixit: > > >I can build nodejs on amhdal.debian.org if you're not comfortable with > that. > > The problem with the DSA porterboxen is that you cannot install your own > built packages in the chroot to use them there… unless there’s a > solution not yet known to me? > Indeed, but the binary can be run from build dir, so I just need to try and reproduce the bug from there.
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Jérémy Lal dixit: >I can build nodejs on amhdal.debian.org if you're not comfortable with that. The problem with the DSA porterboxen is that you cannot install your own built packages in the chroot to use them there… unless there’s a solution not yet known to me? bye, //mirabilos -- “ah that reminds me, thanks for the stellar entertainment that you and certain other people provide on the Debian mailing lists │ sole reason I subscribed to them (I'm not using Debian anywhere) is the entertainment factor │ Debian does not strike me as a place for good humour, much less German admin-style humour”
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
That'd be great, thank you - my local (emulated) aarch64 build of nodejs is proving to be much more time consuming than I expected. On Tue, 28 Feb 2023 at 23:21, Jérémy Lal wrote: > > > > Le mar. 28 févr. 2023 à 19:06, James Addison a écrit : >> >> On Tue, Feb 28, 2023, 17:55 Thorsten Glaser wrote: >>> >>> Can you test it? I don’t have the bandwidth for that right now… >> >> >> Should be able to, yep - I seem to remember seeing some repro instructions >> from you on the GitHub thread and will give those a try in an emulator/vm. > > > I can build nodejs on amhdal.debian.org if you're not comfortable with that. > >> -- >> Pkg-javascript-devel mailing list >> pkg-javascript-de...@alioth-lists.debian.net >> https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-javascript-devel
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Le mar. 28 févr. 2023 à 19:06, James Addison a écrit : > On Tue, Feb 28, 2023, 17:55 Thorsten Glaser wrote: > >> Can you test it? I don’t have the bandwidth for that right now… > > > Should be able to, yep - I seem to remember seeing some repro instructions > from you on the GitHub thread and will give those a try in an emulator/vm. > I can build nodejs on amhdal.debian.org if you're not comfortable with that. -- > Pkg-javascript-devel mailing list > pkg-javascript-de...@alioth-lists.debian.net > > https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-javascript-devel >
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
On Tue, Feb 28, 2023, 17:55 Thorsten Glaser wrote: > Can you test it? I don’t have the bandwidth for that right now… Should be able to, yep - I seem to remember seeing some repro instructions from you on the GitHub thread and will give those a try in an emulator/vm.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >Hi - what do you both think of the attached patch, which brings the ARM stack >size into line with almost all other architectures (= 984 KB)? It might do the job unless arm64 for some reason uses more stack elsewhere as well. Can you test it? I don’t have the bandwidth for that right now… Thanks, //mirabilos -- > Hi, does anyone sell openbsd stickers by themselves and not packaged > with other products? No, the only way I've seen them sold is for $40 with a free OpenBSD CD. -- Haroon Khalid and Steve Shockley in gmane.os.openbsd.misc
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 X-Debbugs-Cc: t...@debian.org > My plan is to rebuild / retest reverse deps before hard freeze. That's a good plan. Do you know whether any of those tests include cases that spin up large (as in: may consume more than 50% of a system's memory) numbers of processes/threads? Context: I've begun worrying about the additional overhead from stack preallocation -- where increasing the stack size might significantly reduce the number of processes that fit in memory while running simultaneously. > Thanks, I'll welcome any patch to start with. FWIW: I am still somewhere between 'do nothing' and 'ok, maybe, after seeing more data that it is a safe increase'. I don't trust myself enough to write any logic/syscall-related changes in a patch but may provide one that updates the constant limits in the relevant header file(s). Thorsten: if you'd like an rlimit-based approach then I think that may be upon you to write, or to request from upstream (where I accidentally impersonated you on the GitHub issue, by the way - sorry about that!).
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Le mar. 28 févr. 2023 à 00:33, James Addison a écrit : > Package: nodejs > Followup-For: Bug #1030284 > X-Debbugs-Cc: t...@debian.org, > reply+aagshfqluldiwi3obwdg6lgb7if7fevbnhheauz...@reply.github.com > > mirabilos gesagt: > > > We know the default ulimits for users in Debian, and they are higher > > than the 1 MiB assumed by v8, by quite some factor, so this won’t break > > things which are not currently broken (by that exception). This will do > > for the release I think. > > Relaying my understanding of this, so far: > > An increase in the V8 stack size should not cause earlier-process-exits > for any > processes that previously ran on Debian systems where the > architecture-default-or-greater stack size is configured[1]. > > In other words: the same-number-or-greater of JavaScript processes should > continue to run on any given Debian system where the configured stack size > is > greater-than-or-equal to the architecture's default, after the V8 stack > size > limit is increased. > > And we expect that it should repair a failing reproducible build test for > at > least one Debian package on arm64. Thanks, I'll welcome any patch to start with. My plan is to rebuild / retest reverse deps before hard freeze.
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 X-Debbugs-Cc: t...@debian.org, reply+aagshfqluldiwi3obwdg6lgb7if7fevbnhheauz...@reply.github.com mirabilos gesagt: > We know the default ulimits for users in Debian, and they are higher > than the 1 MiB assumed by v8, by quite some factor, so this won’t break > things which are not currently broken (by that exception). This will do > for the release I think. Relaying my understanding of this, so far: An increase in the V8 stack size should not cause earlier-process-exits for any processes that previously ran on Debian systems where the architecture-default-or-greater stack size is configured[1]. In other words: the same-number-or-greater of JavaScript processes should continue to run on any given Debian system where the configured stack size is greater-than-or-equal to the architecture's default, after the V8 stack size limit is increased. And we expect that it should repair a failing reproducible build test for at least one Debian package on arm64. [1] - see limits.conf
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
James Addison dixit: >Maybe it's rare to propose 'do nothing' as a technical suggestion but I think >it is worth considering here, since we are not the arbiters of Node. It’s still a release-critical bug in Debian which impacts arm64 builders including reproducible-builds. I would see this fixed in bookworm, at least by a band-aid (raising these limits by default); the proper fix can be revisited after the release, and in coordination with upstream. We know the default ulimits for users in Debian, and they are higher than the 1 MiB assumed by v8, by quite some factor, so this won’t break things which are not currently broken (by that exception). This will do for the release I think. If a proper, upstream-supported, fix arrives within $time it’s even possible that a bookworm-security update includes that. Still thinking along the line of getrlimit + subtract known arch-specific value for the fix here, whatever makes sense with the v8 setting anyway… bye, //mirabilos -- Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~ (as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 Control: forwarded -1 https://github.com/nodejs/node/issues/41163
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 On Wed, 15 Feb 2023 14:54:03 +0100, Jérémy wrote: > While waiting for the proper fix you describe, I propose to set higher > constants > for V8_DEFAULT_STACK_SIZE_KB - especially for arm64. That sounded good to me when I read it a week ago, but now I'm not so sure. It seems that the Node ecosystem has known unusual architecture-specific behaviours here that derive from a lower-level element (V8) in the stack. Is it wise for us to add another layer of configuration settings that are Debian-specific and will require future users/developers yet more time to unpick and understand? Especially when the configuration may -- if set too high -- introduce as many failures as it solves? (how could we tell?) Maybe it's rare to propose 'do nothing' as a technical suggestion but I think it is worth considering here, since we are not the arbiters of Node.
Bug#1030284: [Pkg-javascript-devel] Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Le mer. 15 févr. 2023 à 14:39, Thorsten Glaser a écrit :
> Hi James,
>
> (you might wish to Cc <${bugnumber}-submit...@bugs.debian.org> so they
> actually get the reply…)
>
> >Are you able to determine whether
> https://github.com/nodejs/node/issues/41163
> >(and/or any of the guidance within that thread) seems relevant to this
> bug?
>
> It appears so. I commented there, thank you for finding that link.
>
> I guess there is even a… quick patch… to make from this. I admit
> I’m very confused by that statement:
>
> “if you set it too high, you risk crashes”
>
> That can’t be right.
>
> Searching through the nodejs source for where this stack size is
> set, I see multiple time bombs for all architectures.
>
> deps/v8/src/common/globals.h does set the default stack size to
> 864/984 KiB in order to achieve an about 1 MiB stack for JS plus
> C++ parts combined.
>
> I wonder if this shouldn’t be getrlimit(RLIMIT_STACK) - overhead,
> and then define the per-architecture overhead in a suitable way.
>
> That lower 1 MiB total limit seems to be for Windows. The lower
> arm64 limit is caused by “allocating MacroAssembler takes 120 [KiB]”
> but the total could still be raised I think… at least on unixoid
> platforms other than WebView-on-Android. Since the location of these
> defaults is in v8, it also applies for browsers and whatnot, but
> nodejs could indeed inspect the current ulimit and set a better
> default for at least nōn-Windows systems.
>
> I’m not, unfortunately, in the position to provide a quick patch,
> being a C developer, not CFrustFrust, and all that. I think that
> InitializeNodeWithArgs in src/node.cc, which already has a call
> to V8::SetFlagsFromString(NODE_V8_OPTIONS, …), is the likely place
> for adding code (suitably platform-ifdef’d) that does:
>
> - get the ulimit
> - subtract some arch-specific overhead target
> - check that that’s positive (or >= V8_DEFAULT_STACK_SIZE_KB even,
> that might be a good idea)
> - if so, pass this as synthetic --stack-size (or --stack_size?) to
> v8, overriding its default but allowing for a later option given
> by the user’s argv[] to override _that_, again
>
> Might need to adjust some tests, too :~
While waiting for the proper fix you describe, I propose to set higher
constants
for V8_DEFAULT_STACK_SIZE_KB - especially for arm64.
Jérémy
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Hi James,
(you might wish to Cc <${bugnumber}-submit...@bugs.debian.org> so they
actually get the reply…)
>Are you able to determine whether https://github.com/nodejs/node/issues/41163
>(and/or any of the guidance within that thread) seems relevant to this bug?
It appears so. I commented there, thank you for finding that link.
I guess there is even a… quick patch… to make from this. I admit
I’m very confused by that statement:
“if you set it too high, you risk crashes”
That can’t be right.
Searching through the nodejs source for where this stack size is
set, I see multiple time bombs for all architectures.
deps/v8/src/common/globals.h does set the default stack size to
864/984 KiB in order to achieve an about 1 MiB stack for JS plus
C++ parts combined.
I wonder if this shouldn’t be getrlimit(RLIMIT_STACK) - overhead,
and then define the per-architecture overhead in a suitable way.
That lower 1 MiB total limit seems to be for Windows. The lower
arm64 limit is caused by “allocating MacroAssembler takes 120 [KiB]”
but the total could still be raised I think… at least on unixoid
platforms other than WebView-on-Android. Since the location of these
defaults is in v8, it also applies for browsers and whatnot, but
nodejs could indeed inspect the current ulimit and set a better
default for at least nōn-Windows systems.
I’m not, unfortunately, in the position to provide a quick patch,
being a C developer, not CFrustFrust, and all that. I think that
InitializeNodeWithArgs in src/node.cc, which already has a call
to V8::SetFlagsFromString(NODE_V8_OPTIONS, …), is the likely place
for adding code (suitably platform-ifdef’d) that does:
- get the ulimit
- subtract some arch-specific overhead target
- check that that’s positive (or >= V8_DEFAULT_STACK_SIZE_KB even,
that might be a good idea)
- if so, pass this as synthetic --stack-size (or --stack_size?) to
v8, overriding its default but allowing for a later option given
by the user’s argv[] to override _that_, again
Might need to adjust some tests, too :~
Good luck,
//mirabilos
--
exceptions: a truly awful implementation of quite a nice idea.
just about the worst way you could do something like that, afaic.
it's like anti-design. that too… may I quote you on that?
sure, tho i doubt anyone will listen ;)
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Followup-For: Bug #1030284 Hi Thorsten, Are you able to determine whether https://github.com/nodejs/node/issues/41163 (and/or any of the guidance within that thread) seems relevant to this bug? If so, your repro example could be useful to help upstream/contributors to develop and test a solution. Thanks, James
Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded
Package: nodejs Version: 18.13.0+dfsg1-1 Severity: serious Tags: upstream Justification: breaks on release architecture X-Debbugs-Cc: t...@mirbsd.de Control: affects -1 src:dygraphs During reproducible-builds testing, I found that one of my packages, the one with nodejs used during build, worked on amd64 (which the arch:all packages are built on), i386, armhf(!) but not on arm64. Testing on the porterbox reveals that indeed nodejs fails there. To reproduce, easiest way: $ apt-get source dygraphs # = 2.2.0-4 $ cd dygraphs-2.2.0 $ babeljs --config-file $PWD/babel.config.json --compact false --source-maps inline -d tests5 auto_tests Omitting --compact false and --source-maps inline can also be done, the bug is triggered nevertheless: RangeError: Maximum call stack size exceeded at NodePath.getScope (/usr/share/nodejs/@babel/traverse/lib/path/index.js:84:11) at NodePath.setScope (/usr/share/nodejs/@babel/traverse/lib/path/context.js:115:21) at NodePath.setContext (/usr/share/nodejs/@babel/traverse/lib/path/context.js:128:8) at NodePath.pushContext (/usr/share/nodejs/@babel/traverse/lib/path/context.js:183:8) at TraversalContext.visitQueue (/usr/share/nodejs/@babel/traverse/lib/context.js:78:14) at TraversalContext.visitSingle (/usr/share/nodejs/@babel/traverse/lib/context.js:65:19) at TraversalContext.visit (/usr/share/nodejs/@babel/traverse/lib/context.js:109:19) at traverseNode (/usr/share/nodejs/@babel/traverse/lib/traverse-node.js:18:17) at NodePath.visit (/usr/share/nodejs/@babel/traverse/lib/path/context.js:86:52) at TraversalContext.visitQueue (/usr/share/nodejs/@babel/traverse/lib/context.js:86:16) There are numerous references to nodejs failing in this way on arm64 platforms, normal Linux but even on Android as well, on the internet; h01ger said several other nodejs-using packages FTBFS on arm64 in the same way during reproducible-builds testing. I consider this an architecture-specific release-critical bug. Maybe having a reproducer and access to a porterbox will allow a nodejs maintainer to track this down. -- System Information: Debian Release: bookworm/sid APT prefers unstable-debug APT policy: (500, 'unstable-debug'), (500, 'unstable') merged-usr: no Architecture: arm64 (aarch64) Kernel: Linux 5.10.0-21-arm64 (SMP w/4 CPU threads) Locale: LANG=C, LC_CTYPE=C.UTF-8 (charmap=UTF-8), LANGUAGE not set Shell: /bin/sh linked to /bin/dash Init: unable to detect Versions of packages nodejs depends on: ii libc6 2.36-8 ii libnode108 18.13.0+dfsg1-1 Versions of packages nodejs recommends: ii ca-certificates 20211016 pn nodejs-doc Versions of packages nodejs suggests: pn npm -- no debconf information
