1. 17 Aug, 2020 1 commit
  2. 16 Aug, 2020 3 commits
    • Robin Cheng's avatar
      Fix a race condition in the thread exit/join code in DeterministicSchedule. · 013b287c
      Robin Cheng authored
      Summary:
      Previously, there was a race between exitingSems_[...] = tls.sem (in beforeThreadExit) and exitingSems_[...].post() (in joinAll). The race exists because exitingSems_ is a map that is not protected by the "shared access" synchronization mechanism used by DeterministicSchedule (i.e. it does not sit between a beforeSharedAccess and afterSharedAccess pair). Specifically, the main thread can be reading exitingSems_ while a child thread is writing to it.
      
      This diff fixes that: after all threads are ready to exit (this part isn't changed), we first wait for the joiner thread itself to be scheduled, and then, it will post the semaphore to wake up the child thread to run its thread-local destructors, and at the same time call .join() on the child. All this runs in a "critical section" between beforeSharedAccess and afterSharedAccess in the joiner thread.
      
      For clarity also changed the child thread to use descheduleCurrentThread, and to make the DeterministicSchedule destructor no longer call beforeThreadExit since it makes little sense.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23119075
      
      fbshipit-source-id: 22bc6ffd9a9d888cc09bdee066dcac7f05b29fa7
      013b287c
    • Yedidya Feldblum's avatar
      Reimplement SignalSafeElfCache::Path · 8622363b
      Yedidya Feldblum authored
      Summary: [Folly] Reimplement `SignalSafeElfCache::Path` minimally so that it meets the expectations of `std::basic_string` as used by the rest of `SignalSafeElfCache`.
      
      Reviewed By: chadaustin, luciang
      
      Differential Revision: D23152224
      
      fbshipit-source-id: 48c7a7277d00a928d3de2937699ad112d6d560c2
      8622363b
    • Yedidya Feldblum's avatar
      Cut an extra rdtsc in DistributedMutex spin · bc6736ac
      Yedidya Feldblum authored
      Summary: [Folly] Cut an extra `rdtsc` in `DistributedMutex` spin.
      
      Reviewed By: aary
      
      Differential Revision: D21947987
      
      fbshipit-source-id: 8d6dca75f5290581a886385cefe994c19af839e1
      bc6736ac
  3. 13 Aug, 2020 2 commits
    • Mark Santaniello's avatar
      Introduce FallbackSysArenaAllocator · 8cde5e00
      Mark Santaniello authored
      Summary:
      It's difficult to use C++ allocators if they are not default-constructible.  The non-default-constructibility "infects" the container, and any class that holds the container (transitively).
      
      In D21967640 (https://github.com/facebook/folly/commit/f0471228b89be4a399afcf0e9692c84754d06875), I made `folly::SysArenaAllocator` default constructible, and adopted the behavior of merely throwing `std::bad_alloc` in the event that we didn't *actually* initialize it with a `folly::SysArena`.  In hindsight, it's not clear this was an improvement.  All it did was move a compile-time problem to run-time.
      
      What I do here is a bit better.  I enhance `CxxAllocatorAdaptor` such that it has two modes:
      - **Normal / classic mode:** non-default constructible.
        - Guaranteed at compile-time to have a non-null "inner allocator"
      - **`FallbackToStdAlloc` mode:** default-constructible.
        - Passes allocations through to `std::allocator` if no "inner allocator" provided
      
      Using this support, I introduce a new allocator, `folly::FallbackSysArenaAllocator`, which is similar to `SysArenaAllocator` *except* that it uses the fallback mode and is therefore default-constructible.
      
      This is useful in a large codebase where we want to use arena-allocation on a specific *instance* of a container type -- and not every single instance.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22938039
      
      fbshipit-source-id: feaba12d76564a386a03e759eedf06a77bbc8942
      8cde5e00
    • Xavier Deguillard's avatar
      flush stdout before starting a command while interactive · 56693404
      Xavier Deguillard authored
      Summary:
      Now that the subprocess output is no longer piped, we can see a weird situation
      where the command being run is displayed after the compilation step:
        + cd /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/build/eden && \
        [1/13] rust_job_pool
            Finished release [optimized] target(s) in 0.75s
        [12/13] Install the project...
        -- Install configuration: "RelWithDebInfo"
        -- Installing: /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/bin/edenfs
        -- Set runtime path of "/data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/bin/edenfs" to ""
        -- Up-to-date: /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/bin/edenfsctl
        -- Up-to-date: /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/lib/libbackingstore_rs.a
        -- Up-to-date: /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/lib/libbackingstore.a
        -- Up-to-date: /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/include/eden/scm/lib/backingstore/c_api/HgNativeBackingStore.h
        -- Up-to-date: /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/eden/include/eden/scm/lib/backingstore/c_api/RustBackingStore.h
        + /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/installed/cmake-Ncng4tsJb6gdOu40ggy14-YtgNQD43 (https://github.com/facebook/folly/commit/f47621ec247c8009d8a8aae8823f0de7c27a70fe)k5ev0n-FXq99I/bin/cmake \
        +      --build \
        +      /data/users/xavierd/scratch/dataZusersZxavierdZfbsource/fbcode_builder_getdeps/build/eden \
        +      --target \
        +      install \
        +      --config \
        +      Release \
        +      -j \
        +      24
      
      This is a bit awkward. Flushing stdout's buffer allows for the ordering to be
      correct.
      
      Reviewed By: wez
      
      Differential Revision: D23079405
      
      fbshipit-source-id: e2bf25b098d6ab4a788a5ec07deb635a42cae18c
      56693404
  4. 12 Aug, 2020 7 commits
    • Chris Keeline's avatar
      Back out "Fix fiber backtraces in gdb" · 698754f3
      Chris Keeline authored
      Summary: Original commit changeset: 2d4ad67a4a01
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23064000
      
      fbshipit-source-id: 88d8d550772c2045f61d5a355cf33c9d8df7d046
      698754f3
    • Zach Zundel's avatar
      reclaimbale -> reclaimable · e9ae08b8
      Zach Zundel authored
      Summary: Found because I saw the error message
      
      Reviewed By: magedm
      
      Differential Revision: D23019298
      
      fbshipit-source-id: 6744e9e6d0c21cf76db756f68b3e948a044e6888
      e9ae08b8
    • Dan Melnic's avatar
      Fix TSAN-reported race in ShutdownSocketSet test · 621beb2e
      Dan Melnic authored
      Summary: [Folly] Fix TSAN-reported race in `ShutdownSocketSet` test where the shutdown does not wait for the server.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23057822
      
      fbshipit-source-id: 4b682e6e80cff503e85cb2b663f8399b69265a77
      621beb2e
    • Christopher Gist's avatar
      fix folly::fibers GDB extension for GCC 9 · 1aba9144
      Christopher Gist authored
      Summary:
      The folly::fibers GDB extension, specifically the `info fibers` command,
      depended on a specific format of the std::unique_ptr pretty printer
      output. This pretty printer format changed between GCC 7 and GCC 9, with
      the latter omitting the target address. However, the new pretty printer
      does expose an iterator for the child target address that we can use
      instead.
      
      Differential Revision: D23063364
      
      fbshipit-source-id: 6125edaba9abcfd0b7a9c0741a18854637c97804
      1aba9144
    • Mark Santaniello's avatar
      Eliminate lift_void_to_char · 09a33a57
      Mark Santaniello authored
      Summary:
      `std::allocator<void>::allocate` is not invocable as policy. Let folly allocators behave the same.
      
      Switch existing sites to parameterizing the allocators over `char` instead, since `std::allocator<char>::allocate` is invocable.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22986960
      
      fbshipit-source-id: ca1ff3cb1785029b6b6f59ddc4046b1b105d8bd6
      09a33a57
    • Nathan Bronson's avatar
      fix forwarding for hinted insert_or_assign · efe2962d
      Nathan Bronson authored
      Summary:
      The hinted forms of insert_or_assign were forwarding with
      std::move rather than std::forward, which has the potential to elevate
      a non-const lvalue ref to an rvalue ref.
      
      (Note: this ignores all push blocking failures!)
      
      Reviewed By: yfeldblum
      
      Differential Revision: D21475865
      
      fbshipit-source-id: e91e759762cf6f045c843cd7de1402953e6afb61
      efe2962d
    • Nathan Bronson's avatar
      Back out "suggest race condition to users that trip F14's internal checks" · cc16262f
      Nathan Bronson authored
      Summary: Original commit changeset: 8d5e96feea4d
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23046629
      
      fbshipit-source-id: e24c4a0ba5f40a584646a602f618e881dbf0f344
      cc16262f
  5. 11 Aug, 2020 5 commits
    • Robin Cheng's avatar
      Fix an issue in ElfCache that makes it async-signal-unsafe. · 8b650246
      Robin Cheng authored
      Summary:
      basic_fbstring unfortunately just ignores the allocator. Switching it to use std::basic_string.
      
      This is necessary because ElfCache must be async-signal-safe, but basic_fbstring ignores the allocator and always uses standard malloc/free, which are not safe async-signal-safe.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23035304
      
      fbshipit-source-id: 7b4e77a3764701366556a5fe5a352f952b052ed4
      8b650246
    • Robin Cheng's avatar
      Fix TSAN issues for //folly/io/async/test:async_test · 4506ae37
      Robin Cheng authored
      Summary:
      These are all test setup issues:
       - EventBase needs to be destructed before objects that callbacks may refer to.
       - A socket should not be operated on by a thread that is different from the event base it is associated with (in this case, the socket was being closed on the RSA computation thread).
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23043485
      
      fbshipit-source-id: 733868317aef09e32e79169aedb84a988de3bb41
      4506ae37
    • Pranav Thulasiram Bhat's avatar
      Add blocking logging to fibers::Baton · 73a35da0
      Pranav Thulasiram Bhat authored
      Summary: Increase coverage of blocking operation logging.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22933413
      
      fbshipit-source-id: e820927ddafa0bb5f2d50dbb1dceeeb2e89133b3
      73a35da0
    • Eric Niebler's avatar
      fix a bad comment in folly/Poly.h · e6d5beb4
      Eric Niebler authored
      Summary: A doc comment in folly/Poly.h makes reference to a non-existent entity "`Decay`" and is missing namespace qualifiers, making the example confusing and wrong.
      
      Reviewed By: Alfus, yfeldblum
      
      Differential Revision: D23038865
      
      fbshipit-source-id: 50f515b31919552e11ae18ff645ef71812d77bfb
      e6d5beb4
    • Yedidya Feldblum's avatar
      Simplify reentrant_allocator hierarchy · 94cc2abe
      Yedidya Feldblum authored
      Summary: [Folly] Simplify `reentrant_allocator` hierarchy while preserving presence/absence rules of `allocate` and `dealllocate`. Apply same rules to `max_size` and `address`. Plus some assorted tweaks. Plus some comments.
      
      Reviewed By: luciang
      
      Differential Revision: D23016816
      
      fbshipit-source-id: 601318ea112193afcffc93d6783dbd79168a164e
      94cc2abe
  6. 10 Aug, 2020 3 commits
    • Yedidya Feldblum's avatar
      Add exception_wrapper::from_exception_ptr taking by && · c7d609d4
      Yedidya Feldblum authored
      Summary: [Folly] Add `exception_wrapper::from_exception_ptr` taking by `&&`, permitting callers to move, since `std::exception_ptr` copy is expensive and `std::rethrow_exception` takes by value.
      
      Reviewed By: ericniebler, markisaa
      
      Differential Revision: D23016839
      
      fbshipit-source-id: c07695fef0828a50426fe4e2b40fae9582c7ffd9
      c7d609d4
    • Robin Cheng's avatar
      Fix a previously introduced bug in ElfTests related to PIE and addresses. · ebc1ae68
      Robin Cheng authored
      Summary:
      This failed on mode/opt, because the `- sh_addr + sh_offset` was actually using the address and offset of the *symbol table section*, not the *data section* (where the kStringValue variable actually is). It still worked, because for mapped sections, the `offset - address` just happens to be the same for all sections, so it didn't matter what section we used; in debug mode, we use dynamic linking, so the symbol section is called .dynsym and is loaded in memory, but in opt mode, the symbol section is called .symtab and is *not* loaded, so sh_addr is zero, and that formula no longer works.
      
      The goal there was really just to subtract the binary base address encoded in the ELF file. ElfFile already provides this via getBaseAddress(), and that's also how we can identify whether the loaded ELF (in this case, the same as the current executable) is PIE; so switched both the calculation and the if condition to use this result.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D23020998
      
      fbshipit-source-id: f7e66bb554c62aa13d7d2fc1017cb31542507be9
      ebc1ae68
    • Dan Melnic's avatar
      Avoid traversing the callbacks map twice when inserting a non-existing entry · 38bbc80e
      Dan Melnic authored
      Summary: Avoid traversing the callbacks map twice when inserting a non-existing entry
      
      Reviewed By: kevin-vigor
      
      Differential Revision: D23030404
      
      fbshipit-source-id: afbf7564f4039ffbd942f407429874fde795025d
      38bbc80e
  7. 09 Aug, 2020 1 commit
  8. 08 Aug, 2020 2 commits
  9. 07 Aug, 2020 8 commits
    • Chad Austin's avatar
      Avoid std::string with reentrant_allocator in ElfCache · abb4ece6
      Chad Austin authored
      Summary:
      The implementation of `std::basic_string` with the legacy ABI and with `_GLIBCXX_FULLY_DYNAMIC_STRING`, as appears by default on CentOS 7, incorrectly requires the provided allocator type to have a default constructor. The `Allocator` concept does not require this and many allocator types with runtime state shared between copies will not provide this.
      
      Switch to `folly::fbstring` in `ElfCache` to sidestep this problem.
      
      Fixes: https://github.com/facebook/folly/issues/1344.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22970418
      
      fbshipit-source-id: 1e188d668991d25f53da5702b31f4a7a7ec7d7e8
      abb4ece6
    • Misha Shneerson's avatar
      fix buffer overrun · b19f4306
      Misha Shneerson authored
      Summary: (Note: this ignores all push blocking failures!)
      
      Reviewed By: andriigrynenko
      
      Differential Revision: D23005233
      
      fbshipit-source-id: 4ea528505e06fec0af7841ccbd6d41eabe0de558
      b19f4306
    • Robin Cheng's avatar
      Make //folly/tracing/test:static_tracepoint_test work if compiled as PIE. · d6085636
      Robin Cheng authored
      Summary:
      A tracepoint semaphore address is written to the notes section as either the absolute address (if binary is compiled as non-PIE) or the relative address (if binary is compiled as PIE) of the semaphore. In the former case, the test passes; in the latter case, the test as it was could not possibly pass because the binary is mapped to some random base address.
      
      This diff makes a guess: if the addresses are equal, then we're non-PIE; otherwise check if the address offset is valid - that offset is the address at which the binary is mapped, so it always begins with the ELF header, so we can check against that.
      
      If someone knows how to easily figure out that random address or to detect whether the binary is PIE (thereby disabling the test) that'd be great too.
      
      This fixes TSAN for the test, as TSAN binaries are built as PIE.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22752824
      
      fbshipit-source-id: cd320f9fb8355a20f65e2385f0a1ab1c24ec0b81
      d6085636
    • Sotirios Delimanolis's avatar
      Fix OpenSSLUtils::getCommonName to correctly trim trailing null characters · 4892f1bb
      Sotirios Delimanolis authored
      Summary:
      Previously `OpenSSLUtils::getCommonName` was allocating a 64 byte long string and copying the common name from the `X509` into it. We unexpectedly returned that string potentially containing trailing `\0`s.
      
      This diff rewrites the function to allocate a big enough buffer, write to it, then return an appropriately trimmed new string from it.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22910431
      
      fbshipit-source-id: 77b4fc3ccde4f7ddb0f62b884ecd7ff0c868c1ca
      4892f1bb
    • TJ Yin's avatar
      make time based unit-test less flaky by waiting longer · 85a8672b
      TJ Yin authored
      Reviewed By: yfeldblum, andriigrynenko
      
      Differential Revision: D22965140
      
      fbshipit-source-id: 8f9996ccc28aff621879774e901b0c75e8c62883
      85a8672b
    • Maged Michael's avatar
      hazptr: Make cohort active_ data-race-free · f2d225e4
      Maged Michael authored
      Summary: Make hazptr_obj_cohort active_ atomic to be data-race-free.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22976574
      
      fbshipit-source-id: 5a09aa7cedc26c71b2160b2df14a7e0f7b55e2e7
      f2d225e4
    • Paul Jakma's avatar
      folly/README: Update the Fedora deps in the public README · 0bf08a92
      Paul Jakma authored
      Summary:
      The deps for Fedora in the public README are a little out of date.  Add /
      update some.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22951514
      
      fbshipit-source-id: 645af3df1b71dcdab06408d73d99c34c482ea534
      0bf08a92
    • Chris Keeline's avatar
      Fix fiber backtraces in gdb · 30dcb7d8
      Chris Keeline authored
      Summary:
      When we do the context switch onto the fiber, the stack frame points back to
      the main context stack. While the fiber is running, this allows backtraces to
      continue back onto the main context. When the fiber is suspended, we execute
      code on the main context's stack, the pointers to this stack are now garbage,
      backtracing in gdb runs into problems, such as garbage frames, gdb
      crashes, and occasional cycles.
      
      The fix here is to write a 0 to the saved return address in the last frame, when a fiber suspends.
      gdb detects this and stops the trace
        #38 folly::fibers::FiberManager::activateFiber(folly::fibers::Fiber*) (this=0x0, fiber=0x0) at folly/fibers/FiberManagerInternal-inl.h:82
        #39 folly::fibers::FiberManager::runReadyFiber(folly::fibers::Fiber*) (this=0x0, fiber=0x0) at folly/fibers/FiberManagerInternal-inl.h:127
        #40 0x0000000000000000 in  ()
      
      I gated the fix to linux since I'm not sure about the ABI for other platforms.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22774189
      
      fbshipit-source-id: 2d4ad67a4a01bebc4ff1f9c285f6e3e95d13432c
      30dcb7d8
  10. 06 Aug, 2020 8 commits
    • Cooper Lees's avatar
      Update zstd dev debian package · d0302913
      Cooper Lees authored
      Summary:
      - Move to the newer and forward moving libzstd-dev
      https://packages.ubuntu.com/search?suite=default&section=all&arch=any&keywords=libzstd&searchon=names
      
      Reviewed By: wez
      
      Differential Revision: D22981784
      
      fbshipit-source-id: 3647357aad60f2335ea494c35c174e9ffec61346
      d0302913
    • Robin Cheng's avatar
      Fix an atomic memory ordering issue in SIMD version of the ConcurrentHashMap. · 015949ec
      Robin Cheng authored
      Summary: The find path reads a node that the insert path writes; they need to have acquire/release semantics due to the node being allocated on the fly.
      
      Reviewed By: yfeldblum, magedm
      
      Differential Revision: D22937394
      
      fbshipit-source-id: c1497ed1764cd43e396bc520e9b716cdfe29e5ca
      015949ec
    • Xavier Deguillard's avatar
      runcmd: do not pipe stdout on a tty · bdf5690a
      Xavier Deguillard authored
      Summary:
      Redirecting stdout means that ninja/cmake won't act as if it's invoked
      interactively, ie: it will buffer the output, show every single files being
      compiled (instead of just a line or progress), etc. This results in a fairly
      janky UX when getdeps is used on the command line. By not redirecting stdout,
      we get immediate feedback about the tests being run, and the files being
      compiled.
      
      Reviewed By: wez
      
      Differential Revision: D22967815
      
      fbshipit-source-id: 872ddbf421065686c384a3a876d0acb8832baf2e
      bdf5690a
    • Yedidya Feldblum's avatar
      Prefer std::condition_t vs preprocessor in AsyncSocket · cbe8ef9d
      Yedidya Feldblum authored
      Summary: [Folly] Prefer `std::condition_t` vs preprocessor in `AsyncSocket`.
      
      Reviewed By: bschlinker, lnicco
      
      Differential Revision: D22975614
      
      fbshipit-source-id: 562d1b794ec3dbab538ad507b2ec5d0fe07c7757
      cbe8ef9d
    • Yedidya Feldblum's avatar
      Let exceptionStr peek into unknown exception types · ec55993d
      Yedidya Feldblum authored
      Summary: [Folly] Let `exceptionStr` peek into unknown exception types when building with libstdc++ via `std::exception_ptr::__cxa_exception_type()`.
      
      Reviewed By: ot, luciang
      
      Differential Revision: D22521812
      
      fbshipit-source-id: b75811ed4159df81e651c5df1386c406f6ac6d60
      ec55993d
    • Shai Szulanski's avatar
      Add yield_value(Try<>) to coro::Task · dde95013
      Shai Szulanski authored
      Summary: Generic code taking in a `Task<void|T>` cannot easily forward the result without throwing exceptions because the `return_value` overload taking a `Try` cannot be added to `Task<void>` which already has `return_void`. Instead we use `yield_value` as we did with `co_error` to make the desired generic code possible.
      
      Reviewed By: lewissbaker
      
      Differential Revision: D22934887
      
      fbshipit-source-id: ec50de5832f9038d92749bdde2c1d43244288a3c
      dde95013
    • Dylan Yudaken's avatar
      pre-allocate freeList · 5076d050
      Dylan Yudaken authored
      Summary: We know ahead of time the size of this list, so may as well pre-allocate space for it
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22838412
      
      fbshipit-source-id: 7d70495fe0910e65dfea49fe7baf675308ab9331
      5076d050
    • Robin Cheng's avatar
      Make folly/experimental/symbolizer/test:elf_tests work if compiled as PIE. · e0bd7f09
      Robin Cheng authored
      Summary: If the binary is compiled as PIE, such as in the case of TSAN, the kStringValue symbol's value, which is an address that in turn points to the "coconuts" character array, may be stored as 0 in the binary file, and rely on the dynamic linker to relocate at loading time. The corresponding relocation entry (in the .rela.dyn section) contains the relative offset instead. We do not seem to have implemented parsing of the relocation section, so this diff changes the test to be more lenient, and upon seeing a zero will just verify the address of the symbol against the live binary.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D22782080
      
      fbshipit-source-id: ffbfc7c23b84865fb29bbfb782feb896d687594b
      e0bd7f09