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

Fix data race against EditorFileSystem.scanning_changes_done #88124

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

Rubonnek
Copy link
Member

@Rubonnek Rubonnek commented Feb 9, 2024

Fixes the following report by ThreadSanitizer:

==================
WARNING: ThreadSanitizer: data race (pid=142244)
  Read of size 1 at 0x7b6c002a4b91 by main thread:
    #0 EditorFileSystem::_notification(int) /opt/godot/editor/editor_file_system.cpp:1272:10 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x588d21f) (BuildId: 3b2cca770c49afa3)
    #1 EditorFileSystem::_notificationv(int, bool) /opt/godot/editor/editor_file_system.h:146:2 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x58a530d) (BuildId: 3b2cca770c49afa3)
    #2 Object::notification(int, bool) /opt/godot/core/object/object.cpp:837:3 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0xa16bff3) (BuildId: 3b2cca770c49afa3)
    #3 SceneTree::_process_group(SceneTree::ProcessGroup*, bool) /opt/godot/scene/main/scene_tree.cpp (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x70e5d65) (BuildId: 3b2cca770c49afa3)
    #4 SceneTree::_process(bool) /opt/godot/scene/main/scene_tree.cpp:1028:8 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x70e27b1) (BuildId: 3b2cca770c49afa3)
    #5 SceneTree::process(double) /opt/godot/scene/main/scene_tree.cpp:508:2 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x70e3767) (BuildId: 3b2cca770c49afa3)
    #6 Main::iteration() /opt/godot/main/main.cpp:3939:44 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3ce42aa) (BuildId: 3b2cca770c49afa3)
    #7 OS_LinuxBSD::run() /opt/godot/platform/linuxbsd/os_linuxbsd.cpp:945:7 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3bb4604) (BuildId: 3b2cca770c49afa3)
    #8 main /opt/godot/platform/linuxbsd/godot_linuxbsd.cpp:86:6 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3ba46f4) (BuildId: 3b2cca770c49afa3)

  Previous write of size 1 at 0x7b6c002a4b91 by thread T58:
    #0 EditorFileSystem::_thread_func_sources(void*) /opt/godot/editor/editor_file_system.cpp:1186:29 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x588ca0b) (BuildId: 3b2cca770c49afa3)
    #1 Thread::callback(unsigned long, Thread::Settings const&, void (*)(void*), void*) /opt/godot/core/os/thread.cpp:64:3 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98c47a8) (BuildId: 3b2cca770c49afa3)
    #2 void std::__invoke_impl<void, void (*)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long, Thread::Settings, void (*)(void*), void*>(std::__invoke_other, void (*&&)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long&&, Thread::Settings&&, void (*&&)(void*), void*&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/invoke.h:61:14 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98c4cb3) (BuildId: 3b2cca770c49afa3)
    #3 std::__invoke_result<void (*)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long, Thread::Settings, void (*)(void*), void*>::type std::__invoke<void (*)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long, Thread::Settings, void (*)(void*), void*>(void (*&&)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long&&, Thread::Settings&&, void (*&&)(void*), void*&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/invoke.h:96:14 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98c4cb3)
    #4 void std::thread::_Invoker<std::tuple<void (*)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long, Thread::Settings, void (*)(void*), void*>>::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul>) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/std_thread.h:292:13 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98c4cb3)
    #5 std::thread::_Invoker<std::tuple<void (*)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long, Thread::Settings, void (*)(void*), void*>>::operator()() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/std_thread.h:299:11 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98c4cb3)
    #6 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long, Thread::Settings, void (*)(void*), void*>>>::_M_run() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/std_thread.h:244:13 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98c4cb3)
    #7 execute_native_thread_routine msdf-error-correction.cpp (godot.linuxbsd.editor.dev.x86_64.llvm.san+0xa692ee2) (BuildId: 3b2cca770c49afa3)

  Location is heap block of size 1680 at 0x7b6c002a4600 allocated by main thread:
    #0 malloc <null> (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3b35463) (BuildId: 3b2cca770c49afa3)
    #1 Memory::alloc_static(unsigned long, bool) /opt/godot/core/os/memory.cpp:75:14 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98bbcdd) (BuildId: 3b2cca770c49afa3)
    #2 operator new(unsigned long, char const*) /opt/godot/core/os/memory.cpp:40:9 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x98bbcdd)
    #3 EditorNode::EditorNode() /opt/godot/editor/editor_node.cpp:6456:26 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x5a501e0) (BuildId: 3b2cca770c49afa3)
    #4 Main::start() /opt/godot/main/main.cpp:3578:18 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3cdf856) (BuildId: 3b2cca770c49afa3)
    #5 main /opt/godot/platform/linuxbsd/godot_linuxbsd.cpp:84:6 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3ba46d9) (BuildId: 3b2cca770c49afa3)

  Thread T58 (tid=142443, finished) created by main thread at:
    #0 pthread_create <null> (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3b20336) (BuildId: 3b2cca770c49afa3)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State>>, void (*)()) <null> (godot.linuxbsd.editor.dev.x86_64.llvm.san+0xa692fc9) (BuildId: 3b2cca770c49afa3)
    #2 EditorFileSystem::scan_changes() /opt/godot/editor/editor_file_system.cpp:1227:18 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x588cc60) (BuildId: 3b2cca770c49afa3)
    #3 EditorNode::_notification(int) /opt/godot/editor/editor_node.cpp:740:39 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x5a03f04) (BuildId: 3b2cca770c49afa3)
    #4 EditorNode::_notificationv(int, bool) /opt/godot/editor/editor_node.h:123:2 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x5a7b34d) (BuildId: 3b2cca770c49afa3)
    #5 Object::notification(int, bool) /opt/godot/core/object/object.cpp:837:3 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0xa16bff3) (BuildId: 3b2cca770c49afa3)
    #6 Node::propagate_notification(int) /opt/godot/scene/main/node.cpp:2288:2 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x7087066) (BuildId: 3b2cca770c49afa3)
    #7 Node::propagate_notification(int) /opt/godot/scene/main/node.cpp:2291:12 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x7087093) (BuildId: 3b2cca770c49afa3)
    #8 SceneTree::_notification(int) /opt/godot/scene/main/scene_tree.cpp (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x70fcf3a) (BuildId: 3b2cca770c49afa3)
    #9 SceneTree::_notificationv(int, bool) /opt/godot/scene/main/scene_tree.h:84:2 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x70fcf3a)
    #10 Object::notification(int, bool) /opt/godot/core/object/object.cpp:837:3 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0xa16bff3) (BuildId: 3b2cca770c49afa3)
    #11 DisplayServerX11::process_events() /opt/godot/platform/linuxbsd/x11/display_server_x11.cpp:4660:45 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3bf1c83) (BuildId: 3b2cca770c49afa3)
    #12 OS_LinuxBSD::run() /opt/godot/platform/linuxbsd/os_linuxbsd.cpp:941:35 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3bb45ef) (BuildId: 3b2cca770c49afa3)
    #13 main /opt/godot/platform/linuxbsd/godot_linuxbsd.cpp:86:6 (godot.linuxbsd.editor.dev.x86_64.llvm.san+0x3ba46f4) (BuildId: 3b2cca770c49afa3)

SUMMARY: ThreadSanitizer: data race /opt/godot/editor/editor_file_system.cpp:1272:10 in EditorFileSystem::_notification(int)
==================

Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

I think it makes sense. I'm not entirely sure why the original code didn't have the scanning code do a call_deferred to the main thread.... but it seems to me that as in this PR, polling an atomic bool in NOTIFICATION_PROCESS is roughly the same effect and should be correct.

@Rubonnek Rubonnek force-pushed the fix-efs-scan-done-race branch from 15589c8 to 47be311 Compare February 9, 2024 12:47
@Rubonnek Rubonnek force-pushed the fix-efs-scan-done-race branch from 47be311 to 9790b99 Compare February 9, 2024 12:48
@akien-mga akien-mga merged commit 27e575a into godotengine:master Feb 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Rubonnek Rubonnek deleted the fix-efs-scan-done-race branch February 9, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants