Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update clang on FreeBSD 10 #723

Closed
targos opened this issue May 16, 2017 · 31 comments
Closed

update clang on FreeBSD 10 #723

targos opened this issue May 16, 2017 · 31 comments

Comments

@targos
Copy link
Member

targos commented May 16, 2017

Ref: nodejs/node-v8#1

Latest V8 cannot be compiled with version 3.4.1. We should try to update to 3.4.2 and see if that fixes the crash.

/cc @jbergstroem
@jbergstroem
Copy link
Member

Ok, I will open a PR tomorrow.

@targos
Copy link
Member Author

targos commented May 25, 2017

ping

@jbergstroem
Copy link
Member

Truly sorry :( $wrk, etc etc. I will get to it as soon as I can; likely tomorrow morning.

@targos targos mentioned this issue May 30, 2017
Closed
@targos
Copy link
Member Author

targos commented Jun 13, 2017

ping @nodejs/build (no worries @jbergstroem. I totally understand that you can be busy)

@targos targos mentioned this issue Jun 20, 2017
Closed
@mhdawson mhdawson mentioned this issue Jun 20, 2017
Closed
@Trott Trott mentioned this issue Jun 20, 2017
Closed
@Trott
Copy link
Member

Trott commented Jun 20, 2017

@gibfahn Did you mean to add the wg-agenda label or is ctc-agenda correct? If ctc-agenda, can you (or someone else) provide a short description of what you hope CTC will do? Thanks!

@joaocgreis
Copy link
Member

Was wg-agenda and was already discussed in #766

@kfarnung
Copy link

I'm going to be taking a look at it this week.

@kfarnung
Copy link

@targos I've taken a look at this and it's not too straightforward. I have opened #772 for the first step, installing the clang34 package, but there's a bit more subtlety to the builds on FreeBSD. I'll keep working on this throughout the week.

As a side note, are the builds still failing with 3.4.1? I don't see any indication of the original build issue in the CI or in my local builds.

@targos
Copy link
Member Author

targos commented Jun 27, 2017

@kfarnung Thank you for looking into this.
The builds are failing with V8 6.0. You can try to reproduce with this branch: https://github.com/targos/node/tree/v8-6.0

The latest failure on CI is here: https://ci.nodejs.org/job/node-test-commit-freebsd/9863/nodes=freebsd10-64/console

@kfarnung
Copy link

kfarnung commented Jun 27, 2017 •
edited
Loading

Thanks @targos, that was the key. I'm now able to repro the crash. Unfortunately I'm still seeing the crash on 3.4.2, see attached, so I'm going to try 3.5 (EDIT: also failing) and 3.8 (EDIT: passing) as well, just to see if any of the newer versions resolve the issue.

Attempts

@kfarnung
Copy link

@jbergstroem Is there any objection to upgrading clang to 3.8.1? That's approximately the same version as FreeBSD 11 (which uses 3.8.0).

@kfarnung kfarnung mentioned this issue Jun 27, 2017
Closed
@jbergstroem
Copy link
Member

@kfarnung I will have a chat with package maintainers in FreeBSD seeing how they should/would run into this as well.

@jbergstroem
Copy link
Member

@jbergstroem said:
I will have a chat with package maintainers in FreeBSD seeing how they should/would run into this as well.

I've emailed with @bradleythughes (one of the node maintainers in ports/freebsd) about this and their build system can reproduce the same issues. The idea is to spend a bit of time seeing how feasible it is to fix this upstream (v8).

In general, is the v8 team moving forward with raising the lowest required clang version? @targos do you perhaps know?

@targos
Copy link
Member Author

targos commented Jun 30, 2017

@jbergstroem I don't know if there is a lowest required clang version.
Note that the crash cannot be reproduced with V8 6.1 so you might be able to bisect V8 to find the fix.

@targos targos mentioned this issue Jun 30, 2017
Closed
8 tasks
@MylesBorins
Copy link
Contributor

Can we get this upgrade? This is blocking us from the V8 6.0 release

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

Note that the crash cannot be reproduced with V8 6.1 so you might be able to bisect V8 to find the fix.

Sounds like we need to bisect and then have that fix backported. Upgrading clang sounds good too, but a backport sounds quicker.

@MylesBorins
Copy link
Contributor

@targos is doing this bisect something you can do?
@ofrobots perhaps you can help?

@targos
Copy link
Member Author

targos commented Jul 18, 2017

The error message from nodejs/node-v8#1 (comment) mentions:

3.	../deps/v8/src/wasm/wasm-module.cc:2656:5: Generating code for declaration 'AsyncCompileJob::CompileTask<0>::CompileTask'
c++: error: unable to execute command: Segmentation fault (core dumped)

The code for AsyncCompileJob moved in v8/v8@c7892d3.
IMO it's highly probable that this is what "fixed" the crash. I will try to backport this commit.

@targos
Copy link
Member Author

targos commented Jul 18, 2017

I give up. This file had too many changes between 6.0 and the refactoring...

This was referenced Jul 20, 2017
Closed
Closed
@jbergstroem
Copy link
Member

So, have we established a version we're going with?

@kfarnung
Copy link

If we want to stay with current OS packages then it's 3.8.1 (which is slightly ahead of the FreeBSD 11 machine which uses 3.8.0).

@gibfahn
Copy link
Member

gibfahn commented Jul 22, 2017

3.8.1 SGTM

@jbergstroem
Copy link
Member

Ok, I can do the PR.

@kfarnung
Copy link

You can base it on #772

@MylesBorins
Copy link
Contributor

@targos I dug in a little bit more and it would appear the biggest change (aside from the giant refactor) was making AsyncCompileJob::CompileTask no longer a template class. I'm thinking that the bug in Clang is related to this

@MylesBorins
Copy link
Contributor

I think I have a fix

MylesBorins/node@70b4ec5

@nschonni nschonni mentioned this issue Jul 26, 2017
Closed
@bnoordhuis
Copy link
Member

@MylesBorins We updated the baseline to clang 3.4.2 for precisely the reason you describe in the commit log.

gibfahn added a commit to gibfahn/node that referenced this issue Jul 26, 2017
@gibfahn
build: move FreeBSD to experimental
03419c4 
The FreeBSD platform arguably doesn't have enough dedicated maintainers
to be considered a Tier 2 platform. However I think it does match the
requirements for Experimental:

```
These are often working to be promoted to Tier 2 but are not quite ready.
There is at least one individual actively providing maintenance and the team
is striving to broaden quality and reliability of support.
```

AFAICT Johan (jbergstroem) has been singlehandedly maintaining support.
We get a fair number of FreeBSD specific issues, most recently with V8
6.0 (which I don't think officially supports FreeBSD).

This is intended as a conversation starter, I have no strong feelings
about this. If there are actually a bunch of people in
@nodejs/platform-freebsd who have the time to fix issues (or if I just
didn't notice...) then this can be closed.

To clarify, making FreeBSD experimental doesn't mean that we'll stop
testing our xLinux builds on the platform, it just means that we won't
necessarily be blocked on FreeBSD specific issues on (for example) V8
upgrades.

Refs: nodejs#14384
Refs: nodejs/build#723
@gibfahn gibfahn mentioned this issue Jul 26, 2017
Closed
2 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 26, 2017 via email

@ignisf ignisf mentioned this issue Jul 27, 2017
Closed
@ignisf
Copy link

ignisf commented Aug 23, 2017

@MylesBorins could you please link to your change in upstream V8?

@ignisf
Copy link

ignisf commented Aug 23, 2017

Ah, nvm, found it https://chromium-review.googlesource.com/c/v8/v8/+/585979

@targos
Copy link
Member Author

targos commented Sep 19, 2017

I suppose this can be closed for now.

@targos targos closed this as completed Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@joaocgreis @jbergstroem @bnoordhuis @MylesBorins @Trott @kfarnung @ignisf @targos @gibfahn