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.

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

release-1.6: Backports for Julia 1.6.8 #46116

Closed
wants to merge 54 commits into from

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jul 20, 2022

Backported PRs:

Need manual backport:

Contains multiple commits, manual intervention needed:

Non-merged PRs with backport label:

@KristofferC KristofferC added the release Release management and versioning. label Jul 20, 2022
@KristofferC KristofferC force-pushed the backports-release-1.6 branch 4 times, most recently from 7a4c4b0 to 2ce254a Compare December 21, 2022 12:20
@KristofferC KristofferC requested a review from a team as a code owner December 21, 2022 12:20
@KristofferC

This comment was marked as outdated.

@KristofferC
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs = ":release-1.6")

@nanosoldier

This comment was marked as outdated.

@KristofferC
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":release-1.6")

@nanosoldier
Copy link
Collaborator

Your job failed. cc @maleadt

@maleadt
Copy link
Member

maleadt commented Dec 21, 2022

The build failed because gfortran was removed from the package rootfs, https://github.com/JuliaCI/rootfs-images/, resulting in broken triple detection:

Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ curl: (22) The requested URL returned error: 404
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ make[2]: *** [/source/deps/mbedtls.mk:126: /source/deps/srccache/MbedTLS.v2.24.0+4..tar.gz] Error 22
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ make[2]: *** [/source/deps/pcre.mk:60: /source/deps/srccache/PCRE2.v10.40.0+0..tar.gz] Error 22
Dec 21 08:58:14 amdci8 bash[65957]: [152B blob data]
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ curl: (22) The requested URL returned error: 404
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ make[2]: *** [/source/deps/p7zip.mk:49: /source/deps/srccache/p7zip.v17.4.0+0..tar.gz] Error 22
Dec 21 08:58:14 amdci8 bash[65957]: [111B blob data]
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ curl: (22) The requested URL returned error: 404
Dec 21 08:58:14 amdci8 bash[65957]: [111B blob data]
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ curl: (22) The requested URL returned error: 404
Dec 21 08:58:14 amdci8 bash[65957]: [111B blob data]
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ curl: (22) The requested URL returned error: 404
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ make[2]: *** [/source/deps/mpfr.mk:80: /source/deps/srccache/MPFR.v4.1.1+0..tar.gz] Error 22
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ make[2]: *** [/source/deps/nghttp2.mk:56: /source/deps/srccache/nghttp2.v1.41.0+0..tar.gz] Error 22
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │                                  Dload  Upload   Total   Spent    Left  Speed
Dec 21 08:58:14 amdci8 bash[65957]: [269B blob data]
Dec 21 08:58:14 amdci8 bash[65957]:       From worker 2:        │ curl: (22) The requested URL returned error: 404

Notice the wrong filenames, MPFR.v4.1.1+0..tar.gz.

Maybe we can force this using a build flag; let's try:

@nanosoldier runtests(ALL, vs = ":release-1.6", vs_configuration = (buildflags=["BB_TRIPLET_LIBGFORTRAN_CXXABI=x86_64-linux-gnu-libgfortran5-cxx11"],))

If that doesn't work I'll deploy a workaround.

@DilumAluthge DilumAluthge removed the request for review from a team December 21, 2022 17:36
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@@ -1474,8 +1474,6 @@ static inline uint16_t float_to_half(float param)
return h;
}

#if !defined(_OS_DARWIN_) // xcode already links compiler-rt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KristofferC I'm not sure if you want to merge this commit into your backport or not; it's up to you.

@staticfloat
Copy link
Member

The analyzegc failure looks real: https://buildkite.com/julialang/julia-release-1-dot-6/builds/228#01853669-24f9-461b-8078-efeb7ed89c35/814-1059

git blame shows that @vtjnash is the last person to move around the jl_lock_profile() code, and it's a bit hard for me to tell; should this function be considered a safepoint or not?

@vtjnash
Copy link
Member

vtjnash commented Dec 22, 2022

I don't know about v1.6, but on master, it is annotated

src/julia_internal.h:JL_DLLEXPORT void jl_lock_profile(void) JL_NOTSAFEPOINT;

@staticfloat
Copy link
Member

The windows build now makes it all the way to system image emission, and then dies with an access violation in _ZN12_GLOBAL__N_119WinCOFFObjectWriter11writeObjectERN4llvm11MCAssemblerERKNS1_11MCAsmLayoutE. I can't say I've seen this before.

@staticfloat
Copy link
Member

I don't know about v1.6, but on master, it is annotated

src/julia_internal.h:JL_DLLEXPORT void jl_lock_profile(void) JL_NOTSAFEPOINT;

Yes, that was added by you. It looks like on standard release-1.6, the analyzegc pass fails, so I guess we can just ignore that.

@staticfloat
Copy link
Member

staticfloat commented Dec 22, 2022

The llvmpasses issue is due to some problem in our LLVM builds the assert build does not contain a needed symbol:

$ nm ./libLLVM.v11.0.1+3.x86_64-linux-gnu-cxx11/lib/libLLVM-11jl.so | grep DisableABIBreakingChecks
0000000003e9ec30 B _ZN4llvm24DisableABIBreakingChecksE
$ nm ./libLLVM_assert.v11.0.1+3.x86_64-linux-gnu-cxx11/lib/libLLVM-11jl.so | grep DisableABIBreakingChecks
$

So this may just be a problem with our earlier LLVM builds. Once again, I think this is just something we didn't run much on 1.6. I'm just going to disable llvmpasses and analyzegc on 1.6 for now.

X-ref: JuliaCI/julia-buildkite#272

@staticfloat
Copy link
Member

The windows build now makes it all the way to system image emission, and then dies with an access violation in _ZN12_GLOBAL__N_119WinCOFFObjectWriter11writeObjectERN4llvm11MCAssemblerERKNS1_11MCAsmLayoutE. I can't say I've seen this before.

Update: this only happens when we set JULIA_CPU_TARGET; if we set it to generic, or don't set it, this doesn't happen. But as soon as we include multiversioning (e.g. just setting JULIA_CPU_TARGET='generic;sandybridge,-xsaveopt,clone_all') it breaks. On release-1.6, we can set it to the full typical value of generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1) and it doesn't crash. So something in this backport stack broke it; I will next run a git bisect to try and figure out what broke it.

@KristofferC
Copy link
Member Author

Thanks for all your work here @staticfloat!

@staticfloat
Copy link
Member

staticfloat commented Dec 23, 2022

Bisect points to 93915f8. Manually reverting that on the tip of this branch fixes things. and when I go and look at the original PR, lo and behold, Tim already fixed this in a newer version of LLVM. So I guess the options are we can either bump LLVM version, or we can drop that backport.

@KristofferC
Copy link
Member Author

Let's drop it then.

@staticfloat
Copy link
Member

The Pkg test failure is that Base.runtests() sets --depwarn=error, and the release-1.6 Pkg branch has deprecated generate. Note that the deprecation was reverted, but it appears that the reversion hasn't been backported to Pkg's release-1.6 branch.

@staticfloat
Copy link
Member

I have rebased and dropped the two FP16-related commits.

@staticfloat
Copy link
Member

@KristofferC can I leave it to you to deal with the Pkg errors? I think most likely the right thing to do is to backport the generate improvements to 1.6.

dkarrasch and others added 13 commits October 10, 2023 15:27
This should prevent LinearAlgebra from trying to increase our OpenBLAS
thread count in its `__init__()` method when we're not trying to enable
threaded BLAS.

(cherry picked from commit a8b3994)
(cherry picked from commit 8830c26)
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 626f3b2)
This was causing our rewriting of the loader's RPATH emulation to fail
after running `make install`, as MSYS2 was rewriting our list that looks
like:

```
../bin/libgcc_s_seh-1.dll:../bin/libopenlibm.dll:@../bin/libjulia-internal.dll:@../bin/libjulia-codegen.dll:
```

Into one that looked like:
```
..\bin\libgcc_s_seh-1.dll;..\bin\libopenlibm.dll;@..\bin\libjulia-internal.dll;@..\bin\libjulia-codegen.dll;
```

By exporting `MSYS2_ARG_CONV_EXCL='*'` for all `stringreplace`
invocations, we dodge this issue, as documented by MSYS2 [0].

[0] https://www.msys2.org/wiki/Porting/#filesystem-namespaces
Tragically, I believe MSYS2 is messing with options such as
`/VERYSILENT` turning them instead into `C:\msys2\VERYSILENT` or
similar.
* Makefile: MSYS2: close path conversion for `DEP_LIBS`

Automatic path conversion will replace `:` with `;`

* Makefile: MSYS2: use `cygpath` for path convert

ref: https://www.msys2.org/docs/filesystem-paths/#manual-unix-windows-path-conversion

* devdoc/win/msys2: add build steps

* devdoc/win/msys2: Add x86/x64 build notes

* devdoc/win/msys2: apply sugestions

Co-Authored-By: Elliot Saba <staticfloat@gmail.com>

* Instead of `CC_WITH_ENV`, scope environment variables to targets

* Fix whitespace

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
This allows our `llvm-config.exe` to find its libraries, which is
necessary on Windows since there's no `RPATH` support.

This was backported as a small piece of 53603f6
…7968)

On later versions of Julia, we have the ability to set CPUs for our
subprocesses more flexibly, but on v1.6 we don't, so let's ignore these
tests for now.

X-ref: 32b1305
Our new CI environment defines environment variables that break this
test.  We have better behaviors on newer Julia versions.
KristofferC and others added 9 commits October 11, 2023 13:24
(cherry picked from commit c46834b)
* Generalize Bool parse method to AbstractString

Fixes JuliaStrings/InlineStrings.jl#57.

We currently have a specialization for `parse(Bool, ::Union{String, SubString{String})`
where `true` and `false` are parsed appropriately. The restriction to
`Union{String, SubString{String}}`, however, means we don't get this behavior
for other `AbstractString`s. In the linked issue above, for InlineStrings, we end up
going through the generic integer parsing codepath which results in an `InexactError`
when we try to do `Bool(10)`.

The proposal in this PR takes advantage of the fact that there is only the 2 comparisons
where we do `_memcmp` that require the input string to be "dense" (in memory), and otherwise,
we just do a comparison against a `SubString` of the input string.

Relatedly, I've wanted to introduce the concept of an abstrac type like:

```julia
abstract type MemoryAddressableString <: AbstractString
```

where the additional required interface would be being able to call
`pointer(::MemoryAddressableString)`, since a lot of our string algorithms depend on doing
these kind of pointer operations and hence makes it quite a pain to implement your own
custom string type.

* Apply suggestions from code review

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>
(cherry picked from commit 63830a6)
(cherry picked from commit f6b5157)
(cherry picked from commit 6d678fe)
The dynamic linker needs to unify `llvm::Any::TypeId` across DSOs. In our case
`libjulia-codegen` and `libLLVM`.

See https://github.com/llvm/llvm-project/blob/2bc4c3e920ee078ef2879b00c40440e0867f0b9e/llvm/include/llvm/ADT/Any.h#L30

Fixes: #49121
(cherry picked from commit d8fa3c8)
@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Dec 4, 2023

Is the libcurl CVE serious? libcurl is not just for Pkg, also Downloads?

I.e. should this just be released, as possibly, the last 1.6 LTS?

I did not look carefully at the errors (false alarms?):

Read : 502 MB ==> 32%ERROR: LoadError: ArgumentError: size too large for standard header: size (0o2360151053530)

@PallHaraldsson
Copy link
Contributor

Simply close this because of new TLS? Or release last point release for some reason (anyone stuck on 1.6.x)?

@benz0li
Copy link
Contributor

benz0li commented Oct 28, 2024

@PallHaraldsson IMHO yes, simply close.

@benz0li
Copy link
Contributor

benz0li commented Oct 28, 2024

@KristofferC Please close [to avoid any possible confusion].

Cross reference:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Release management and versioning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.