This is an archive of the discontinued LLVM Phabricator instance.

Switch std::call_once to llvm::call_once
ClosedPublic

Authored by krytarowski on Jan 30 2017, 9:36 AM.

Details

Summary

The std::call_once implementation in libstdc++ has problems on few systems: NetBSD, OpenBSD and Linux PPC. LLVM ships with a homegrown implementation llvm::call_once to help on these platforms.

This change is required in the NetBSD LLDB port. std::call_once with libstdc++ results with crashing the debugger.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Jan 30 2017, 9:36 AM

I can build and test this patch on NetBSD/amd64.

I don't have access right now to a performant FreeBSD, Linux, Android, Windows and FreeBSD hosts to test build and execute tests for this patch on other platforms. Please check.

I was in touch with libstdc++ developers and the reason why std::call_once crashes is still cryptic. A combination of TLS, shared library and pointer to a function passed to pthread_once(3) results in a crash. Switch LLDB to the standard LLVM wrapper for std::cal_once.

Few months back discussed with @mehdi_amini in the context of LLVM.

krytarowski edited the summary of this revision. (Show Details)Jan 30 2017, 9:42 AM
mehdi_amini edited edge metadata.Jan 30 2017, 9:44 AM

I'm fine with this change, but I'll leave the approval to one of the LLDB developer :)

(Thanks for following-up with the libstdc++ on these platforms!)

clayborg requested changes to this revision.Jan 30 2017, 9:55 AM

Be very careful when using this, you can't change member variables that used to be std::once to be statics. We also don't need the llvm namespace to be included with "using namespace llvm;" in many of the files.

include/lldb/Core/Debugger.h
376 ↗(On Diff #86302)

There must be an in ivar for m_clear_once. This can't be a static. Debugger::Clear() can only be called once per debugger instance, not just once ever.

source/Core/Debugger.cpp
68 ↗(On Diff #86302)

Why was this added? Remove if not needed.

768–769 ↗(On Diff #86302)

This is wrong, it must be in ivar. We are trying to make sure Debugger::Clear() gets calls only once for each instance of a Debugger. Read the comment.

source/Core/ModuleList.cpp
33 ↗(On Diff #86302)

Why was this using added? Remove please.

source/Host/common/Editline.cpp
27 ↗(On Diff #86302)

Why was this added? Remove if not needed? I hope the LLVM_DEFINE_ONCE_FLAG doesn't require the using directive. If it does, it should be fixed.

source/Host/common/HostInfoBase.cpp
33 ↗(On Diff #86302)

Remove

source/Host/linux/HostInfoLinux.cpp
22 ↗(On Diff #86302)

Remove

source/Host/windows/HostInfoWindows.cpp
24 ↗(On Diff #86302)

Remove

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
38 ↗(On Diff #86302)

Remove

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
47 ↗(On Diff #86302)

Remove

source/Plugins/Language/Go/GoLanguage.cpp
30 ↗(On Diff #86302)

Remove

source/Plugins/Language/Java/JavaLanguage.cpp
32 ↗(On Diff #86302)

Remove

source/Plugins/Language/ObjC/ObjCLanguage.cpp
39 ↗(On Diff #86302)

Remove

source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
51 ↗(On Diff #86302)

Remove

source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
51 ↗(On Diff #86302)

Remove

source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
53 ↗(On Diff #86302)

Remove

source/Plugins/Process/POSIX/ProcessPOSIXLog.cpp
24 ↗(On Diff #86302)

Remove

source/Plugins/Process/Windows/Common/ProcessWindows.cpp
44 ↗(On Diff #86302)

Remove

source/Plugins/Process/Windows/Common/ProcessWindowsLog.cpp
20 ↗(On Diff #86302)

Remove

source/Plugins/Process/elf-core/ProcessElfCore.cpp
40 ↗(On Diff #86302)

rm

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
48 ↗(On Diff #86302)

rm

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
90 ↗(On Diff #86302)

rm

source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp
24 ↗(On Diff #86302)

rm

source/Plugins/Process/mach-core/ProcessMachCore.cpp
50 ↗(On Diff #86302)

rm

source/Plugins/Process/minidump/ProcessMinidump.cpp
35 ↗(On Diff #86302)

rm

source/Plugins/ScriptInterpreter/None/ScriptInterpreterNone.cpp
24 ↗(On Diff #86302)

rm

source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
58 ↗(On Diff #86302)

rm

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
91 ↗(On Diff #86302)

rm

558–559 ↗(On Diff #86302)

You can't change a member variable std::once_flag to a static. Change back

1635 ↗(On Diff #86302)

Revert since we shouldn't include llvm namespace.

1641 ↗(On Diff #86302)

Revert since we shouldn't include llvm namespace.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
307 ↗(On Diff #86302)

You can't remove a member variable once flag. Restore with LLVM version.

source/Symbol/GoASTContext.cpp
34 ↗(On Diff #86302)

rm

source/Target/Language.cpp
28 ↗(On Diff #86302)

rm

This revision now requires changes to proceed.Jan 30 2017, 9:55 AM

Be very careful when using this, you can't change member variables that used to be std::once to be statics. We also don't need the llvm namespace to be included with "using namespace llvm;" in many of the files.

using namespace llvm; is currently required in order to get this functional:

  enum InitStatus { Uninitialized = 0, Wait = 1, Done = 2 };
  typedef volatile sys::cas_flag once_flag;
  /// This macro is the only way you should define your once flag for LLVM's
  /// call_once.
#define LLVM_DEFINE_ONCE_FLAG(flag) static once_flag flag = Uninitialized

sys::cas_flag is a member of the llvm namespace.

namespace llvm {
  namespace sys {
    void MemoryFence();

#ifdef _MSC_VER
    typedef long cas_flag;
#else
    typedef uint32_t cas_flag;
#endif
    cas_flag CompareAndSwap(volatile cas_flag* ptr,
                            cas_flag new_value,
                            cas_flag old_value);
  }
}
  • include/llvm/Support/Atomic.h of LLVM

Be very careful when using this, you can't change member variables that used to be std::once to be statics. We also don't need the llvm namespace to be included with "using namespace llvm;" in many of the files.

using namespace llvm; is currently required in order to get this functional:

What about just updating the macro in LLVM to make it not needed?

#define LLVM_DEFINE_ONCE_FLAG(flag) static llvm::once_flag flag = Uninitialized

Be very careful when using this, you can't change member variables that used to be std::once to be statics. We also don't need the llvm namespace to be included with "using namespace llvm;" in many of the files.

using namespace llvm; is currently required in order to get this functional:

What about just updating the macro in LLVM to make it not needed?

#define LLVM_DEFINE_ONCE_FLAG(flag) static llvm::once_flag flag = Uninitialized

Right, this looks cleaner. I will propose this patch as a separated review.

krytarowski updated this revision to Diff 87109.Feb 4 2017, 2:17 PM
krytarowski edited edge metadata.

Revamp patch after recent changes in LLVM.

Related https://reviews.llvm.org/D29552

I'm requesting help to test this patch on !NetBSD with and without LLVM_THREADING_USE_STD_CALL_ONCE defined in "llvm/Support/Threading.h". This issue is blocking me from adding functional changes for the NetBSD port.

labath added inline comments.Feb 4 2017, 5:26 PM
include/lldb/Core/Debugger.h
379 ↗(On Diff #87120)

The code in llvm says you should only ever use the LLVM_DEFINE_ONCE_FLAG to declare flags. I am guessing it's because it forces the static keyword into the declaration, which in turn guarantees your object will be zero-initialized by the linker. If you declare the flag as a local variable like this, it will be initialized to a random value, and you will have a fun time debugging issues in the future (it will only affect netbsd, as std::call_once platforms will still be correctly initialized).

source/Core/ModuleList.cpp
648 ↗(On Diff #87120)

This could be simplified to:

static ModuleList *g_shared_module_list = new ModuleList();

It's only written this way because MSVC at one point did not support thread-safe statics.

krytarowski added inline comments.Feb 4 2017, 6:47 PM
include/lldb/Core/Debugger.h
379 ↗(On Diff #87120)

I was thinking about it, there is m_clear_once() in lldb/source/Core/Debugger.cpp used in the initializer. But apparently there is no zeroing it with this.

Similar case is in DWARFDataSegment.

struct DWARFDataSegment {
    llvm::once_flag m_flag;
    lldb_private::DWARFDataExtractor m_data;
  };

And another one with static llvm::ManagedStatic<llvm::once_flag> g_once_flag; (I don't fully understand the impact here).

Is there an option to turn llvm::once_flag to behave like std::once_flag? For example changing once_flag to a class like:

class once_flag
{
once_flag() : status(Uninitialized) {};
llvm::sys::cas_flag status;
};

and replace occurrences of flag with flag.status.

Perhaps it would change the code to mechanical replacement of std::once_flag and std::call_ones to llvm:: versions.

I've tried to build the LLDB code with mechanically * replaced std::call_once -> llvm::call_once and std::once_flag -> llvm::once_flag:

--- /public/llvm/include/llvm/Support/Threading.h	2017-02-05 00:15:00.769574623 +0100
+++ /usr/pkg/include/llvm/Support/Threading.h	2017-02-05 04:14:03.334251557 +0100
@@ -62,12 +62,16 @@
 
   /// This macro is the only way you should define your once flag for LLVM's
   /// call_once.
-#define LLVM_DEFINE_ONCE_FLAG(flag) static ::llvm::once_flag flag
+#define LLVM_DEFINE_ONCE_FLAG(flag) static once_flag flag
 
 #else
 
   enum InitStatus { Uninitialized = 0, Wait = 1, Done = 2 };
-  typedef volatile sys::cas_flag once_flag;
+  class once_flag {
+  public:
+    once_flag() : status(::llvm::Uninitialized) {};
+    volatile ::llvm::sys::cas_flag status;
+  };
 
   /// This macro is the only way you should define your once flag for LLVM's
   /// call_once.
@@ -96,24 +100,24 @@
 #else
     // For other platforms we use a generic (if brittle) version based on our
     // atomics.
-    sys::cas_flag old_val = sys::CompareAndSwap(&flag, Wait, Uninitialized);
+    sys::cas_flag old_val = sys::CompareAndSwap(&flag.status, Wait, Uninitialized);
     if (old_val == Uninitialized) {
       std::forward<Function>(F)(std::forward<Args>(ArgList)...);
       sys::MemoryFence();
       TsanIgnoreWritesBegin();
-      TsanHappensBefore(&flag);
-      flag = Done;
+      TsanHappensBefore(&flag.status);
+      flag.status = Done;
       TsanIgnoreWritesEnd();
     } else {
       // Wait until any thread doing the call has finished.
-      sys::cas_flag tmp = flag;
+      sys::cas_flag tmp = flag.status;
       sys::MemoryFence();
       while (tmp != Done) {
-        tmp = flag;
+        tmp = flag.status;
         sys::MemoryFence();
       }
     }
-    TsanHappensAfter(&flag);
+    TsanHappensAfter(&flag.status);
 #endif
   }
  • there is one exception, I don't understand:
const DWARFDataExtractor &
SymbolFileDWARF::GetCachedSectionData(lldb::SectionType sect_type,
                                      DWARFDataSegment &data_segment) {
#if 0
  llvm::call_once(data_segment.m_flag, &SymbolFileDWARF::LoadSectionData, this,
                 sect_type, std::ref(data_segment.m_data));
#else
  llvm::call_once(data_segment.m_flag,
    [this, sect_type, &data_segment] {
      this->LoadSectionData(sect_type, std::ref(data_segment.m_data));
    }
  );
#endif
  return data_segment.m_data;
}

This exception is valid for so far all versions of switches out of std::once_flag.

Test results for the above patch for LLVM and LLDB with "mechanical" switch to llvm::call_once.

===================
Test Result Summary
===================
Test Methods:       1224
Reruns:                1
Success:             268
Expected Failure:     21
Failure:             324
Error:               166
Exceptional Exit:      0
Unexpected Success:    1
Skip:                441
Timeout:               3
Expected Timeout:      0

Results from Jan 21st

===================
Test Result Summary
===================
Test Methods:       1218
Reruns:                1
Success:             264
Expected Failure:     20
Failure:             323
Error:               166
Exceptional Exit:      1
Unexpected Success:    1
Skip:                440
Timeout:               3
Expected Timeout:      0

It looks good to me. The patch on review with LLVM_DEFINE_ONCE_FLAG badly timeouts for me and I interrupted it.

Once someone could explain to me why SymbolFileDWARF::GetCachedSectionData cannot be switched mechanically to llvm::call_once, I would like to propose this revamped interface to llvm.

krytarowski updated this revision to Diff 87153.Feb 5 2017, 9:22 AM

Revamp to new llvm::once_flag

See inlined comment about initialization when llvm::once_flag is a member variable

include/lldb/Core/Debugger.h
379 ↗(On Diff #87153)

Is there a valid default initializer for the llvm::once_flag? If so, we are good to go, if not, we need to either initialize it manually, probably by adding another macro to llvm to do the initialization. Static versions might rely on the compiler zeroing out the memory...

krytarowski added inline comments.Feb 6 2017, 9:02 AM
include/lldb/Core/Debugger.h
379 ↗(On Diff #87153)

Yes, there is a valid default initializer for llvm::once_flag.

Related entry in LLVM - D29566

clayborg accepted this revision.Feb 6 2017, 9:07 AM

Thanks for doing the right thing in LLVM first, looks great!

This revision is now accepted and ready to land.Feb 6 2017, 9:07 AM
labath accepted this revision.Feb 6 2017, 9:59 AM
krytarowski closed this revision.Feb 6 2017, 10:06 AM
This revision was automatically updated to reflect the committed changes.