src: add support for ETW stack walking by jdapena · Pull Request #46203 · nodejs...

Test failures are related.

We should probably formulate a strategy on NODE_OPTIONS vis-a-vis V8 options because there are tons of them. Adding them haphazardly is, let's say, suboptimal.

Having said that, --perf-basic-prof is already allowed so why not --enable-etw-stack-walking too?

Test failures are related.

We should probably formulate a strategy on NODE_OPTIONS vis-a-vis V8 options because there are tons of them. Adding them haphazardly is, let's say, suboptimal.

Most interesting thing is that passing any existing V8 flag in command line is already supported, and it will detect if the flag exists. But NODE_OPTIONS has stronger checks.

One possibility would be doing the same thing that is done with CLI arguments and accept any V8 flag. Another possibility is having something like --v8-flags="..." or similar that will pass the arguments to V8. It would be interesting to know why the environment checks are added (if it is just warning user about unsupported flags or security or what...).

Having said that, --perf-basic-prof is already allowed so why not --enable-etw-stack-walking too?

Yes, provisionally we can land this patch (with the extra documentation change I just added). And maybe create a new ticket for working the NODE_OPTIONS V8 flags processing.

or security

That's the reason. With "formulating a strategy" I meant thinking about what flag categories should and should not be accepted.

Right now it's decided on a case-by-case basis but that often leaves glaring holes, like how --max-old-space-size=... was accepted but not --max-semi-space-size=... (fixed, but only recently.)

As first time contributor to Node, I cannot kick workflows for the updated patch

About creating a strategy for V8 flags... my first question is if this patch should wait for that.

Then, about how to implement an strategy (i.e. reworking NODE_OPTIONS processing to be more explicit that we have an "allow list", and which flags are allowed). And about functionality discussion... maybe we want to have some clear rules about what should be allowed or not.

About creating a strategy for V8 flags... my first question is if this patch should wait for that.

No, that's for other maintainers to worry about. :-)

I cannot kick workflows for the updated patch

I'm kind of surprised by that because CI went through just fine the first time... I can start it manually but that's pointless when it's going the fail in the exact same way. Can you run make test locally and fix up the failures?

Landed in b88628c

I opened #46339 for the question:

We should probably formulate a strategy on NODE_OPTIONS vis-a-vis V8 options because there are tons of them. Adding them haphazardly is, let's say, suboptimal.