Skip to content

Merge remote-tracking branch 'NixOS/master' into lazy-trees #4

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
Draft
wants to merge 1 commit into
base: lazy-trees
Choose a base branch
from
Draft

Merge remote-tracking branch 'NixOS/master' into lazy-trees #4

wants to merge 1 commit into from

Conversation

@bb010g
Copy link

@bb010g bb010g commented Jan 11, 2023

figured i'd share this; think I got the merge correct? this merge includes the new error throwing code.

@bb010g bb010g requested a review from edolstra as a code owner January 11, 2023 08:38
@bb010g
Copy link
Author

bb010g commented Jan 11, 2023

This is way less relevant now, but from reviewing git diff bb010g/lazy-trees..edolstra/lazy-trees, there are a few places I think you missed with regards to errors (e.g. src/nix/flake.cc). I'd recommend comparing the behavior of this PR's src/libexpr/primops/patch.cc in particular; I think switching over to state.error gives better diagnostics. (General thing I noticed: Primops are currently called with noPos, so including that in traces will cause an unknown location to be reported. The primop call site is automatically added to any trace, so primops only need to worry about including other positions in their error traces.) src/libcmd/installables.cc line 428 caught my eye also; this line was specifically changed in 1123c42 in response to code review, but your merge reverted that. Anyways, the majority of our changes were the same, which is neat. Feel free to close this whenever you like.

@Artturin
Copy link

@bb010g Eelco has been keeping the branch very up to date recently so this can be closed

I checked that the merge revert has been fixed.

@bb010g
Copy link
Author

bb010g commented Sep 23, 2023

@bb010g Eelco has been keeping the branch very up to date recently so this can be closed

I checked that the merge revert has been fixed.

I'll close this after checking the thoroughness of the current lazy-trees error handling (I remember Eelco's merge being a bit less through than mine when diffing); thanks for the reminder. That'll be in a separate PR anyhow.

@bb010g bb010g marked this pull request as draft September 23, 2023 17:13
edolstra pushed a commit that referenced this pull request Sep 29, 2025
@Mic92
libutil/file-descriptor: handle EAGAIN in read/write operations
2790f5f 
We now see exception beeing thrown when remote building in master
because of writing to a non-blocking file descriptor from our json logger.

> #0  0x00007f2ea97aea9c in __pthread_kill_implementation () from /nix/store/wn7v2vhyyyi6clcyn0s9ixvl7d4d87ic-glibc-2.40-36/lib/libc.so.6
> NixOS#1  0x00007f2ea975c576 in raise () from /nix/store/wn7v2vhyyyi6clcyn0s9ixvl7d4d87ic-glibc-2.40-36/lib/libc.so.6
> #2  0x00007f2ea9744935 in abort () from /nix/store/wn7v2vhyyyi6clcyn0s9ixvl7d4d87ic-glibc-2.40-36/lib/libc.so.6
> #3  0x00007f2ea99e8c2b in __gnu_cxx::__verbose_terminate_handler() [clone .cold] () from /nix/store/ybjcla5bhj8g1y84998pn4a2drfxybkv-gcc-13.3.0-lib/lib/libstdc++.so.6
> #4  0x00007f2ea99f820a in __cxxabiv1::__terminate(void (*)()) () from /nix/store/ybjcla5bhj8g1y84998pn4a2drfxybkv-gcc-13.3.0-lib/lib/libstdc++.so.6
> #5  0x00007f2ea99f8275 in std::terminate() () from /nix/store/ybjcla5bhj8g1y84998pn4a2drfxybkv-gcc-13.3.0-lib/lib/libstdc++.so.6
> #6  0x00007f2ea99f84c7 in __cxa_throw () from /nix/store/ybjcla5bhj8g1y84998pn4a2drfxybkv-gcc-13.3.0-lib/lib/libstdc++.so.6
> #7  0x00007f2eaa5035c2 in nix::writeFull (fd=2, s=..., allowInterrupts=true) at ../unix/file-descriptor.cc:43
> #8  0x00007f2eaa5633c4 in nix::JSONLogger::write (this=this@entry=0x249a7d40, json=...) at /nix/store/4krab2h0hd4wvxxmscxrw21pl77j4i7j-gcc-13.3.0/include/c++/13.3.0/bits/char_traits.h:358
> #9  0x00007f2eaa5658d7 in nix::JSONLogger::logEI (this=<optimized out>, ei=...) at ../logging.cc:242
> #10 0x00007f2ea9c5d048 in nix::Logger::logEI (ei=..., lvl=nix::lvlError, this=0x249a7d40) at /nix/store/a7cq5bqh0ryvnkv4m19ffchnvi8l9qx6-nix-util-2.27.0-dev/include/nix/logging.hh:108
> #11 nix::handleExceptions (programName="nix", fun=...) at ../shared.cc:343
> #12 0x0000000000465b1f in main (argc=<optimized out>, argv=<optimized out>) at /nix/store/4krab2h0hd4wvxxmscxrw21pl77j4i7j-gcc-13.3.0/include/c++/13.3.0/bits/allocator.h:163
> (gdb) frame 10
> #10 0x00007f2ea9c5d048 in nix::Logger::logEI (ei=..., lvl=nix::lvlError, this=0x249a7d40) at /nix/store/a7cq5bqh0ryvnkv4m19ffchnvi8l9qx6-nix-util-2.27.0-dev/include/nix/logging.hh:108
> 108             logEI(ei);

So far only drainFD sets the non-blocking flag on a "readable" file descriptor,
while this is a "writeable" file descriptor.
It's not clear to me yet, why we see logs after that point, but it's
also not that bad to handle EAGAIN in read/write functions after all.
edolstra added a commit that referenced this pull request Sep 29, 2025
@edolstra
nix daemon: Don't open the store
9590167 
This makes it behave the same as nix-daemon. Opening the store in the
parent can cause a SIGBUS in libsqlite in the child:

  #0  0x00007f141cf6f789 in __memset_avx2_unaligned_erms () from /nix/store/wn7v2vhyyyi6clcyn0s9ixvl7d4d87ic-glibc-2.40-36/lib/libc.so.6
  NixOS#1  0x00007f141c322fe8 in walIndexAppend () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #2  0x00007f141c3711a2 in pagerWalFrames () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #3  0x00007f141c38317e in sqlite3PagerCommitPhaseOne.part.0 () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #4  0x00007f141c383555 in sqlite3BtreeCommitPhaseOne.part.0 () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #5  0x00007f141c384797 in sqlite3VdbeHalt () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #6  0x00007f141c3b8f60 in sqlite3VdbeExec () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #7  0x00007f141c3bbfef in sqlite3_step () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #8  0x00007f141c3bd0e5 in sqlite3_exec () from /nix/store/bbd59cqw259149r2ddk4w1q0lr2fch8c-sqlite-3.46.1/lib/libsqlite3.so.0
  #9  0x00007f141da140e0 in nix::SQLiteTxn::commit() () from /nix/store/1m4r8s7s1v54zq9isncvjgia02bffxlz-determinate-nix-store-3.1.0/lib/libnixstore.so
  #10 0x00007f141d9ce69c in nix::LocalStore::registerValidPaths(std::map<nix::StorePath, nix::ValidPathInfo, std::less<nix::StorePath>, std::allocator<std::pair<nix::StorePath const, nix::ValidPathInfo> > > const&)::{lambda()NixOS#1}::operator()() const () from /nix/store/1m4r8s7s1v54zq9isncvjgia02bffxlz-determinate-nix-store-3.1.0/lib/libnixstore.so
edolstra added a commit that referenced this pull request Sep 29, 2025
@edolstra
Restore fixupBoehmStackPointer
3ba1038 
This was removed in NixOS#11152. However,
we need it for the multi-threaded evaluator, because otherwise Boehm
GC will crash while scanning the thread stack:

  #0  GC_push_all_eager (bottom=<optimized out>, top=<optimized out>) at extra/../mark.c:1488
  NixOS#1  0x00007ffff74691d5 in GC_push_all_stack_sections (lo=<optimized out>, hi=<optimized out>, traced_stack_sect=0x0) at extra/../mark_rts.c:704
  #2  GC_push_all_stacks () at extra/../pthread_stop_world.c:876
  #3  GC_default_push_other_roots () at extra/../os_dep.c:2893
  #4  0x00007ffff746235c in GC_mark_some (cold_gc_frame=0x7ffee8ecaa50 "`\304G\367\377\177") at extra/../mark.c:374
  #5  0x00007ffff7465a8d in GC_stopped_mark (stop_func=stop_func@entry=0x7ffff7453c80 <GC_never_stop_func>) at extra/../alloc.c:875
  #6  0x00007ffff7466724 in GC_try_to_collect_inner (stop_func=0x7ffff7453c80 <GC_never_stop_func>) at extra/../alloc.c:624
  #7  0x00007ffff7466a22 in GC_collect_or_expand (needed_blocks=needed_blocks@entry=1, ignore_off_page=ignore_off_page@entry=0, retry=retry@entry=0) at extra/../alloc.c:1688
  #8  0x00007ffff746878f in GC_allocobj (gran=<optimized out>, kind=<optimized out>) at extra/../alloc.c:1798
  #9  GC_generic_malloc_inner (lb=<optimized out>, k=k@entry=1) at extra/../malloc.c:193
  #10 0x00007ffff746cd40 in GC_generic_malloc_many (lb=<optimized out>, k=<optimized out>, result=<optimized out>) at extra/../mallocx.c:477
  #11 0x00007ffff746cf35 in GC_malloc_kind (bytes=120, kind=1) at extra/../thread_local_alloc.c:187
  #12 0x00007ffff796ede5 in nix::allocBytes (n=<optimized out>, n=<optimized out>) at ../src/libexpr/include/nix/expr/eval-inline.hh:19

This is because it will use the stack pointer of the coroutine, so it
will scan a region of memory that doesn't exist, e.g.

  Stack for thread 0x7ffea4ff96c0 is [0x7ffe80197af0w,0x7ffea4ffa000)

(where 0x7ffe80197af0w is the sp of the coroutine and 0x7ffea4ffa000
is the base of the thread stack).

We don't scan coroutine stacks, because currently they don't have GC
roots (there is no evaluation happening in coroutines). So there is
currently no need to restore the other parts of the original patch,
such as BoehmGCStackAllocator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bb010g @Artturin