nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 100 forks source link

Use official `llvm-project` monorepo, expand LLDB versions check #389

Closed trxcllnt closed 2 years ago

trxcllnt commented 3 years ago

Fixes https://github.com/nodejs/llnode/issues/350

simllll commented 3 years ago

Looking forward to this.. can we use this fork in the meantime somehow? is it published somewhere?

trxcllnt commented 3 years ago

@simllll it's not published anywhere, but you can see how we're installing it in our development container here:

No9 commented 3 years ago

Tested this today and it worked as expected @nodejs/diagnostics what can I do to help land this?

mhdawson commented 3 years ago

@mmarchini any chance you can help with the CI issues?

No9 commented 2 years ago

Looks like we need this PR to land first https://github.com/mmarchini/setup-node/pull/1 @mmarchini I see you're on vacation at the moment so understand if you can't get to this soon but do you think we should look at pulling your setup-node into this org?

mmarchini commented 2 years ago

The mirror option really should be implemented upstream for setup-node :(

There are way to many conflicts on my fork, let me see if I can get a PR going on setup-node. In the meantime, I'll disable the non-fixed major CIs so this PR should be unblocked.

No9 commented 2 years ago

Hey @mmarchini Did you start working on the PR to actions/setup-node? If you haven't I can look at picking it up.

From what I can see it's these two commits that need reintegrating but let me know if there is anything else or hidden gotchas. https://github.com/mmarchini/setup-node/commit/6a399da24b33138ea0de9981a95b7e7123f7b1dd https://github.com/mmarchini/setup-node/commit/772e9574c06048dd733148cd1cb8052c1395726b

If they don't accept it we can kick of this process https://github.com/nodejs/admin/blob/main/transfer-repo-into-the-org.md

No9 commented 2 years ago

Update: I've merged the changes from https://github.com/mmarchini/setup-node/commits/mirror into https://github.com/No9/setup-node/tree/mirror I've also applied it to my llnode fork and reapplied the v15 settings. https://github.com/No9/llnode/runs/4206002518?check_suite_focus=true The change seems to be working as it's picking up the mirror code and runs the tests. However the change is failing due to verifyObject no longer working in 14 and I think this is reported here https://github.com/nodejs/llnode/issues/375 I'll continue to work on the failing test.

No9 commented 2 years ago

Update: I've raised this PR to land a green build https://github.com/nodejs/llnode/pull/398 that we can use as a base to land this. This PR should probably also add llvm 10 and 11 to the test definitions https://github.com/nodejs/llnode/blob/main/.github/workflows/push.yml#L32

trxcllnt commented 2 years ago

@No9 This PR should probably also add llvm 10 and 11 to the test definitions

Will do!

codecov-commenter commented 2 years ago

Codecov Report

Merging #389 (5ea886a) into main (39b38a9) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   77.14%   77.18%   +0.03%     
==========================================
  Files          34       34              
  Lines        5023     5027       +4     
==========================================
+ Hits         3875     3880       +5     
+ Misses       1148     1147       -1     
Impacted Files Coverage Δ
test/plugin/scan-test.js 98.97% <ø> (ø)
test/common.js 86.84% <100.00%> (+0.28%) :arrow_up:
test/plugin/workqueue-test.js 100.00% <100.00%> (ø)
src/llv8.cc 71.94% <0.00%> (+0.13%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

No9 commented 2 years ago

Hey @trxcllnt I've picked up merge rights this week and pulled in the CI fix that this was dependent on. I know it's been a while but do you want to rebase and then we can get this merged? I understand we've lost some momentum but it would be great to get it in as it's an important update.

trxcllnt commented 2 years ago

@No9 Sure, I'd be happy to!

trxcllnt commented 2 years ago

Seems like some LLVM versions aren't packaged by Ubuntu. Should I switch the CI runner to use the LLVM apt repositories listed at https://apt.llvm.org/?

No9 commented 2 years ago

My first thought is to add mutually exclusive conditionals based on ${{ matrix.llvm }} here https://github.com/nodejs/llnode/pull/389/files#diff-f3fc934cf0d89bdf07de358896ff90f1202585df812cf606206d1830a9949811R44 Something along the lines of if [ "${{ matrix.llvm }}" -lt "10" ]; then use distro llvm if [ "${{ matrix.llvm }}" -gt "9" ]; then Use llvm repos`

I'm conscious that distros tend to do some magic when they are packaging and as that's the most common use case I think it would be a good idea to continue to catch it.

That said, if I've missed a reason why we should default to all llvm I'm happy to hear it.

trxcllnt commented 2 years ago

@No9 it looks like there's a few specific combos of matrix.os + matrix.llvm that don't have packages in universe, so I'll update the job to only use the LLVM repos for those.

No9 commented 2 years ago

Hey @trxcllnt Took a look at the failing tests tonight. Looks like later versions of lldb are now returning more lines than expected and these are being truncated. Also the content of the response has changed.

So lldb 10 to 14 is returning

error: expression failed to parse:
error: <user expression 16>:1:1: use of undeclared identifier 'invalid_expr'
invalid_expr

Instead of

error: error: use of undeclared identifier 'invalid_expr'

This is compounded by the logic in flush truncating lines on carriage returns https://github.com/nodejs/llnode/blob/main/test/common.js#L60 So for 10-14 the only line that is being returned to the test is the ambiguous

error: expression failed to parse:

I've updated my local version of flush to make an exception for this case

      let line = '';
      let index = 0;

      let inv = buf.indexOf('invalid_expr');
      if (inv !== -1) { 
          line = buf;
          index = buf.length;
        } else {

        index = buf.indexOf('\n');

        if (index === -1)
          break;

        line = buf.slice(0, index);
      }

      buf = buf.slice(index + 1);

And also updated the test to just match on use of undeclared identifier 'invalid_expr' https://github.com/nodejs/llnode/blob/main/test/plugin/inspect-test.js#L661 https://github.com/nodejs/llnode/blob/main/test/plugin/scan-test.js#L30 I don't think this impacts the integrity of the tests and seems to work.

Very conscious that you've put a lot of work in to this so wanted to give you the option of applying something similar to this PR. If you don't have the bandwidth and want me to do it I'm also happy to pick it up.

trxcllnt commented 2 years ago

@No9 feel free to push any fixes directly to this branch! Lemme know if you need access. Whatever we need to do to get this passing.

No9 commented 2 years ago

Given this PR has been open for 16 months and the last major change was 6 weeks ago and we have an approval I am going to merge this. @trxcllnt Thank you so much for all your work if you are ever in Dublin Ireland hit me up for a [insert favourite drink]