hawkinsw added a comment.

There are *so* many reasons that I love this patch.

First and foremost: I am a professor of computer science at University of 
Cincinnati and teaching Programming Languages this semester and I am about to 
teach *this very technique* for protecting against use-after-free errors. 
Having seen this patch I now have a great example of where it is used in real 
life! So, thank you!

Second, I have left some comments but please take what I said with a grain of 
salt. I hope that it is helpful and not a waste of time.

Thank you!
Will



================
Comment at: lldb/source/Symbol/CompilerType.cpp:35
+  bool unused;
+  if (GetTypeSystemGraveyard().Lookup(m_type_system, unused)) {
+    lldbassert(false && "CompilerType belongs to deleted Typesystem");
----------------
I am sorry if this is obvious, but is `CompilerType` used in a multithreaded 
environment? So, is there a possibility that we could pass the check on line 32 
but become invalid by the use of `m_type_system` here and fall victim to an 
attempt (in `Lookup`, perhaps?) to dereference a `NULL` pointer? Again, I am 
sorry if that is a stupid question!


================
Comment at: lldb/source/Symbol/CompilerType.cpp:36
+  if (GetTypeSystemGraveyard().Lookup(m_type_system, unused)) {
+    lldbassert(false && "CompilerType belongs to deleted Typesystem");
+    return false;
----------------
I know that this is a nit, but should we write `TypeSystem`?


================
Comment at: lldb/source/Symbol/TypeSystem.cpp:22
+  // Intentionally leaked to avoid C++ destructor ordering issue.
+  static TypeSystemSet *g_typesystem_graveyard = new TypeSystemSet();
+  return *g_typesystem_graveyard;
----------------
I spent *way* too long researching to make sure that this initialization is 
thread safe and wanted to drop a link here in case anyone is wondering the same 
thing:

[C++ standard: stmt.dcl para 3, sentence 
3](http://eel.is/c++draft/stmt.dcl#3.sentence-3)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136650/new/

https://reviews.llvm.org/D136650

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to