Broken on macOS Catalina?

[dandavison]

Broken on macOS Catalina?

[dawidd6]

Just as I did in the PR 👍

[dandavison]

Thanks very much @uraimo @kevinnio @dawidd6 for all your help here.

brew install git-delta from the public repo should now work on all MacOS versions.

To anyone reading this: previously, delta was in a personal homebrew "tap". To remove this, do

brew untap dandavison/delta

[kevinnio]

Then let's fix it here. :challenge-accepted:

[kevinnio]

Building delta against my manually-upgraded rust-onig and bumping syntect to 3.3.0 produces a working binary.

Cargo.toml

structopt = "0.2.18"
- syntect = "3.2.0"
+ syntect = "3.3.0"
unicode-segmentation = "1.3.0"

+ [patch.crates-io]
+ onig = { path = "../rust-onig/onig" }
+ onig_sys = { path = "../rust-onig/onig_sys" }

[dependencies.error-chain]
version = "0.12.1"

cargo update -p onig -p onig_sys
cargo install --root="." --path="." --force --locked
PAGER="bin/delta" git log -p

[kgadek]

Same for me. Tried brew install --build-from-source git-delta but that didn't help. When I've cloned the repo & cargo build --release, I got a working binary. Built from 788fd34 (current master).

Interestingly brew install --HEAD git-delta doesn't work for me.

[uraimo]

Ok, @dandavison you are right, same issue with syntect/oniguruma:

testrepo % rust-lldb /usr/local/Cellar/git-delta/0.0.13/bin/delta
(lldb) command script import "/usr/local/Cellar/rust/1.38.0/lib/rustlib/etc/lldb_rust_formatters.py"
(lldb) type summary add --no-value --python-function lldb_rust_formatters.print_val -x ".*" --category Rust
(lldb) type category enable Rust
(lldb) target create "/usr/local/Cellar/git-delta/0.0.13/bin/delta"
Current executable set to '/usr/local/Cellar/git-delta/0.0.13/bin/delta' (x86_64).
(lldb) settings set target.input-path inputdiff.txt
(lldb) process launch
Process 93959 launched: '/usr/local/Cellar/git-delta/0.0.13/bin/delta' (x86_64)
Process 93959 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x00000001001397a2 delta`match_at + 1250
delta`match_at:
->  0x1001397a2 <+1250>: jmpq   *(%r10)
    0x1001397a5 <+1253>: movq   -0xb8(%rbp), %r12
    0x1001397ac <+1260>: movl   %r12d, %r14d
    0x1001397af <+1263>: movq   -0x168(%rbp), %rcx
Target 0: (delta) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x00000001001397a2 delta`match_at + 1250
    frame #1: 0x000000010013fa40 delta`onig_search_with_param + 2160
    frame #2: 0x00000001000eb6a1 delta`onig::Regex::search_with_param::h072776aad9161683 + 145
    frame #3: 0x00000001000f4173 delta`syntect::parsing::parser::ParseState::parse_line::h58cc383b4908be58 + 3075
    frame #4: 0x00000001000eba1a delta`syntect::easy::HighlightLines::highlight::hf9b425d80a8cbf39 + 42
    frame #5: 0x0000000100014ae6 delta`delta::delta::handle_hunk_line::hf8d232199f0e8fd6 + 438
    frame #6: 0x0000000100012787 delta`delta::delta::delta::hb0878d00ad829793 + 2327
    frame #7: 0x0000000100010cc8 delta`delta::main::h87f395f932cbd476 + 3480
    frame #8: 0x0000000100017e25 delta`std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h8449c28cd18577ab + 21
    frame #9: 0x000000010016a3c8 delta`std::panicking::try::do_call::h625c6e428a7d9d6a (.llvm.11534170799014444278) + 24
    frame #10: 0x000000010017a11f delta`__rust_maybe_catch_panic + 31
    frame #11: 0x000000010016c983 delta`std::rt::lang_start_internal::h0b5ba3ceb2f387d6 + 179
    frame #12: 0x0000000100011969 delta`main + 41
    frame #13: 0x00007fff6e08d405 libdyld.dylib`start + 1

[kevinnio]

FYI I managed to build a working bat binary by cloning onig and upgrading oniguruma to master manually.

sharkdp/bat#680 (comment)

[kevinnio]

Just pushed a PR to rust-onig. Let's see what the guys over there have to say.

[uraimo]

Same here, building from master works, submitting a formula update to 0.0.13 now.

[uraimo]

Ok, upgrading the formula is not enough since 0.0.13 doesn't work either, only building manually with cargo build with or without debugging info (so, can't even debug this).
Also, the tag doesn't matter, 0.0.12, 0.0.13 and master all work when built manually.

[uraimo]

All I can get from lldb with the homebrew version (always the same match_at segfault, the caller changes):

testrepo % rust-lldb delta
(lldb) command script import "/usr/local/Cellar/rust/1.38.0/lib/rustlib/etc/lldb_rust_formatters.py"
(lldb) type summary add --no-value --python-function lldb_rust_formatters.print_val -x ".*" --category Rust
(lldb) type category enable Rust
(lldb) target create "delta"
Current executable set to 'delta' (x86_64).
(lldb) settings set target.input-path inputdiff.txt
(lldb) process launch
Process 37934 launched: '/usr/local/bin/delta' (x86_64)
Process 37934 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGTTOU
    frame #0: 0x00000001000f0ca0 delta`syntect::parsing::syntax_definition::MatchPattern::regex::h1ad05c0125da658f
delta`syntect::parsing::syntax_definition::MatchPattern::regex::h1ad05c0125da658f:
->  0x1000f0ca0 <+0>: pushq  %rbp
    0x1000f0ca1 <+1>: movq   %rsp, %rbp
    0x1000f0ca4 <+4>: pushq  %r15
    0x1000f0ca6 <+6>: pushq  %r14
Target 0: (delta) stopped.
(lldb) c
Process 37934 resuming
Process 37934 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x000000010013a402 delta`match_at + 1250
delta`match_at:
->  0x10013a402 <+1250>: jmpq   *(%r10)
    0x10013a405 <+1253>: movq   -0xb8(%rbp), %r12
    0x10013a40c <+1260>: movl   %r12d, %r14d
    0x10013a40f <+1263>: movq   -0x168(%rbp), %rcx
Target 0: (delta) stopped.

[kevinnio]

Why would there be any difference between building on homebrew and building manually? 🤔

[dandavison]

Thanks! Let's get to the bottom of this. Is anyone having a problem on Mojave or previous MacOS versions, or is this just a problem on Catalina?

cc @dawidd6 do you have any insight here?

Before it went on public homebrew, there were instructions to install from my homebrew tap. Do these work for people for whom it is broken on public homebrew?

brew uninstall --force git-delta
brew tap dandavison/delta https://github.com/dandavison/delta                                                                                                                                                                              
brew install dandavison/delta/git-delta                           

(To get homebrew into an unconfusing state after trying different tap/public brew installs, I think we can do

brew untap dandavison/delta
brew uninstall --force git-delta
brew install git-delta

)

[dawidd6]

Wish I could help ya, but I don't have access to any macOS system.

[uraimo]

Your tap works (i.e. the binaries built by travis and added to the release work) on Catalina.
No idea on what homebrew could be doing differently when it builds its binaries.

[kevinnio]

Doing cargo install --path delta/ doesn't work either. The generated binary fails the same way as the brew install git-delta one. Only cargo build --release works. I'm on Catalina, btw.

[dawidd6]

I'll submit a PR for that then.

[dandavison]

@dawidd6 Can we replace the public formula with the working formula from the repo? https://github.com/dandavison/delta/blob/master/HomeBrewFormula/git-delta.rb

(Also, the formula in the delta repo prints out instructions for configuring delta which I think are helpful.)

[dawidd6]

We can't use precompiled binaries in homebrew-core. But I can attach those instructions as caveats.

[dandavison]

bat is another rust project on Homebrew: https://github.com/Homebrew/homebrew-core/blame/master/Formula/bat.rb

The only obvious (to me) difference there is that the delta formula has uses_from_macos "llvm". Shall we try without that line?

[uraimo]

uses_from_macos "llvm". Shall we try without that line?

Tried, no changes, I'm now trying with a "build --release" step followed by an "install" step.
(Or we could just copy the binaries over and be done with it)

[kevinnio]

Copy the binaries over seems to be the best. The install step rebuilds them broken.

[dawidd6]

Sorry folks, but other maintainers spoke and they said it should be fixed upstream. Homebrew/homebrew-core#45328 (comment)

[dandavison]

Thanks very much everyone for all the help here!

Then let's fix it here. :challenge-accepted:

👍

OK, so.

• Has anyone managed to reproduce this issue on their own machine (e.g. by performing the same cargo install step as the public Homebrew formula uses?)

[uraimo]

Has anyone managed to reproduce this issue on their own machine (e.g. by performing the same cargo install step as the public Homebrew formula uses?)

Yep, exactly the same cargo install --root /usr/local/Cellar/git-delta/0.0.13 --path . from the git repo, same bug. BUT, adding --debug to the above produces a working binary...

[kevinnio]

Just ran my own tests and I can confirm @uraimo's comment.

[dandavison]

OK, whereas for me on Mojave (10.14.5) that yields a working binary.

I wonder, I see syntect featured in the segfault stack trace, and bat uses syntect. If you do the same cargo install for bat on Catalina, does that binary syntax-highlight correctly?

In fact, I haven't read these carefully yet but I'm assuming they are not irrelevant...

sharkdp/fd#498
sharkdp/bat#680

[uraimo]

The caller always changes (I mean, completely different method), I got that syntect just that time for example.

[kevinnio]

After doing the same cargo install (no debug) to bat, bat test.rb works coorectly.

But bat -p test.rb fails with a segmentation fault:

Trying to use STDIN makes bat raise a SegmentationFault as well:

[uraimo]

Quoting myself from the bat -p thread:

While I can't add much on the cause behind the crash, a thing I've noticed is that the latest version of rust-onig (v5.0.0) still points to oniguruma:83572e983928243d741f61ac290fc057d69fefc3 from April 23rd, way older than the latest release 6.9.3 tagged in August (and that contains fixes for two CVEs and a lot of fixes found by a fuzzer).
Master right now is pointing to oniguruma:565e5f43c2f9fb9905819ca3eb76114aea267890 for 21st of August.
Maybe someone could test a modified version of the rust wrapper?

I don't do Rust and have no idea if we can do a quick test with a rust-onig with the updated dependency to see if something changes...

[dandavison]

Thanks a lot @kevinnio! I wonder if what's ultimately going to be required is a new release of oniguruma? Hopefully someone is going to be able to say exactly what's going on, so that there's confidence that whatever changed in oniguruma since the last release is correct.

[kevinnio]

Yeah, we most likely need a new release. Hopefully, my PR sparks discussion about it in that repo.

[dandavison]

@dawidd6 it sounds like we might be able to do the same as Homebrew/homebrew-core#46670. WDYT?

[dawidd6]

If someone could test it and it would actually fix the issue, I'm all in.

[dandavison]

Right, if anybody is able to do that that would be fantastic and much appreciated. (I'm not on Catalina and can't really upgrade right now.)

[dandavison]

@uraimo @kevinnio Here's a branch of homebrew-core containing an update to the git-delta formula: https://github.com/dandavison/homebrew-core/blob/git-delta-catalina-fix/Formula/git-delta.rb

Would either of you be able to confirm that with this change it produces a functioning binary on MacOS Catalina? That branch contains just the following commit: dandavison/homebrew-core@6aa82c1

[kevinnio]

@dandavison Thanks! I can confirm homebrew is able to build a working binary by using the changes in your branch. Now we just need to merge it into their master branch.

[dandavison]

@kevinnio thanks a lot! New homebrew PR: Homebrew/homebrew-core#47506