Remove std::move from fs wrapper to work around -D_LIBCPP_DEBUG=1 bug #24295

1. 
   MarcoFalke commented at 9:20 AM on February 9, 2022: member

   Fix #24290

2. Remove std::move from fs wrapper to work around bug faf7a61483
3. MarcoFalke added this to the milestone 23.0 on Feb 9, 2022
4. fanquake requested review from ryanofsky on Feb 9, 2022
5. 
   hebasto commented at 10:57 AM on February 9, 2022: member

   Fix #24290

   Out of curiosity, how was this solution found?

6. MarcoFalke added the label Refactoring on Feb 9, 2022
7. 
   MarcoFalke commented at 11:39 AM on February 9, 2022: member

   As there are steps to reproduce, it is possible to reduce the test case. Once the test case is reduced, it is possible to modify the remaining code to see which modifications fix the bug.

8. 
   laanwj commented at 11:40 AM on February 9, 2022: member

   Yes, it would be good to know what the problem is that it works around. Did we make a mistake? If so, how does this fix it. If not, is there an upstream bug in a c++ library? (if so is it reported anywhere)

   Edit: what I'd essentially like to know is "do we need to worry about removing std::move in other places too?"

9. 
   MarcoFalke commented at 11:45 AM on February 9, 2022: member

   This is an upstream bug (https://github.com/llvm/llvm-project/issues/53669). Until it is fixed, we need to work around it for affected libc++ versions. This only affects DEBUG=1 builds with libc++ (-D_LIBCPP_DEBUG=1), which I am not sure if anyone is using outside of tests?

10. MarcoFalke renamed this:
    Remove std::move from fs wrapper to work around bug
    Remove std::move from fs wrapper to work around -D_LIBCPP_DEBUG=1 bug
    on Feb 9, 2022
11. 
    MarcoFalke commented at 11:47 AM on February 9, 2022: member

    (Changed title to clarify -D_LIBCPP_DEBUG=1)

12. 
    laanwj commented at 1:26 PM on February 9, 2022: member

    Ok, thanks for the explanation. Concept ACK! Might want to add a code comment to that effect, that it's a workaround for a LLVM bug. Or some other way of keeping track so that this can be reverted when the workaround is no longer needed.

13. 
    MarcoFalke commented at 1:29 PM on February 9, 2022: member

    Ok, I'll create an issue or pull request after the upstream bug is fixed (which may never happen), or adjust this pull request if the bug is fixed before merge.

14. 
    laanwj commented at 1:45 PM on February 9, 2022: member

    (which may never happen)

    LLVM tends to take bugs really seriously, especially crash bugs. That we can only reproduce it with debug flags doesn't mean it's not corrupting things behind the scenes otherwise?

15. in src/fs.h:37 in faf7a61483

      31 | @@ -32,9 +32,9 @@ class path : public std::filesystem::path
      32 |      using std::filesystem::path::path;
      33 |  
      34 |      // Allow path objects arguments for compatibility.
      35 | -    path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {}
      36 | -    path& operator=(std::filesystem::path path) { std::filesystem::path::operator=(std::move(path)); return *this; }
      37 | -    path& operator/=(std::filesystem::path path) { std::filesystem::path::operator/=(std::move(path)); return *this; }
      38 | +    path(std::filesystem::path path) : std::filesystem::path::path{path} {}
      39 | +    path& operator=(std::filesystem::path path) { std::filesystem::path::operator=(path); return *this; }
      40 | +    path& operator/=(std::filesystem::path path) { std::filesystem::path::operator/=(path); return *this; }
    

    ---

    ---

    

    ryanofsky commented at 8:04 PM on February 9, 2022:

    In commit "Remove std::move from fs wrapper to work around bug" (faf7a61483a959c1a45d5bf7d304809f506a2ce6)

    If you want to avoid more moves here, you should change the std::filesystem::path path) arguments to const std::filesystem::path path). So for example if someone writes

    path1 = path2 / path3;
    

    Current code moves from the (path2 / path3) temporary, and commit faf7a61483a959c1a45d5bf7d304809f506a2ce6 still moves from that temporary. With const reference operator=, the temporary can be copied from instead of moved from, which seems like what we would want to be safe from the bug

16. ryanofsky approved
17. 
    ryanofsky commented at 8:08 PM on February 9, 2022: member

    This change looks safe so code review ACK faf7a61483a959c1a45d5bf7d304809f506a2ce6, but this problem seems pretty deep and I wonder if the workaround is sufficient to prevent it. It seems to avoid crashes with D_LIBCPP_DEBUG we might need to avoid moves for many other long strings, not just path strings in this one place.

18. 
    MarcoFalke commented at 9:00 PM on February 9, 2022: member

    this problem seems pretty deep and I wonder if the workaround is sufficient to prevent it. It seems to avoid crashes with D_LIBCPP_DEBUG we might need to avoid moves for many other long strings, not just path strings in this one place.

    I've also observed unexplainable segmentation faults in the RPC docs, which use std::move on std::string, so it seems likely that this affects anything that uses basic_string (or stuff that builds on it) like filesystem.

19. luke-jr changes_requested
20. 
    luke-jr commented at 3:01 AM on February 12, 2022: member

    Can you add a comment?

21. 
    MarcoFalke commented at 8:57 AM on February 16, 2022: member

    No longer working on this

22. MarcoFalke closed this on Feb 16, 2022

    ---

23. MarcoFalke deleted the branch on Feb 16, 2022
24. DrahtBot locked this on Feb 16, 2023

Contributors

MarcoFalkehebastolaanwjryanofskyluke-jr

Milestone
23.0