Remove undefined behavior around the use of StateType

[shafik]

Summary

lldb::StateType is an enum without an explicit underlying type.

According to [dcl.enum]p8 http://eel.is/c++draft/dcl.enum#8 and [expr.static.cast]p10 http://eel.is/c++draft/expr.static.cast#10 we are limited to setting StateType to values bound with a value range based on the values of the enumerators. Currently we have a test and SWIG interface that either explicitly set StateType to a value outside of the allowed range or in the case of SWIG does not prevent it.

In both cases using UBSan generates a runtime error when we load a value outside of the range allowed by the standard.

This change refactors StateType to keep track of the largest value and range checking to the SWIG interface and removes the invalid test case.

[jingham]

I wonder if it is worth asserting if a process ever returns a state of kNumStateType? I don't think it is necessary in things like StateIsStoppedState, somebody might have found their way here from the SB API's and we want to be forgiving there. But wherever we've done process->GetState(), that should never come back with kNumStateTypes. We already have eStateInvalid to mean "The debugger is confused about the state of the process". So there's never a reason why a process should report its state as kNumStateTypes.

[jingham]

Other than that, this looks fine to me.

[aprantl]

include/lldb/lldb-enumerations.h

60 ↗

(On Diff #163140)

I think we usually do something like eLastsState = eStateSuspended. This avoid having to add the case to all the switches.

scripts/Python/python-typemaps.swig

89 ↗

(On Diff #163140)

nice!
There's an extra space before the ;

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp

779 ↗

(On Diff #163140)

Shouldn't this value trigger an error of sorts in all of these switch statements?

[shafik]

@jingham I am switching to the @aprantl suggestions which feels cleaner and removes this issue.

include/lldb/lldb-enumerations.h

60 ↗

(On Diff #163140)

This approach seems like a much better idiom, I will switch to that.

[shafik]

Switching enum guard to kLastStateType which references the last valid enum which lead to cleaner code instead of inventing a new value which does not have a good meaning in many cases.

[aprantl]

Thanks! I assume there is a whole bunch of other enums for which we should be doing the same thing now?
Also there might be some 1-based ones for which we also need to check the lower bound.

[jingham]

In D51445#1220905, @aprantl wrote:

Thanks! I assume there is a whole bunch of other enums for which we should be doing the same thing now?
Also there might be some 1-based ones for which we also need to check the lower bound.

Be careful about enums in lldb-enumerations.h since they do make it into the SB API's. I don't think the terminating element matters because Python doesn't actually know these are enums, it just thinks they are global variables, but you'd have to make sure you don't mess up other C++ clients.

lldb/trunk/include/lldb/lldb-enumerations.h

enum StateType {
   // should only be signaled as public state changes, and not private state changes.
   eStateStopped,   ///< Process or thread is stopped and can be examined.
   eStateRunning,   ///< Process or thread is running and can't be examined.
   eStateStepping,  ///< Process or thread is in the process of stepping and can
                    ///not be examined.
   eStateCrashed,   ///< Process or thread has crashed and can be examined.
   eStateDetached,  ///< Process has been detached and can't be examined.
   eStateExited,    ///< Process has exited and can't be examined.
-  eStateSuspended  ///< Process or thread is in a suspended state as far
+  eStateSuspended, ///< Process or thread is in a suspended state as far
                    ///< as the debugger is concerned while other processes
                    ///< or threads get the chance to run.
+  kLastStateType = eStateSuspended  
 };
 
 //----------------------------------------------------------------------
 // Launch Flags
 //----------------------------------------------------------------------
 FLAGS_ENUM(LaunchFlags){
     eLaunchFlagNone = 0u,
     eLaunchFlagExec = (1u << 0),  ///< Exec when launching and turn the calling

lldb/trunk/scripts/Python/python-typemaps.swig

%typemap(in) lldb::tid_t {
   }
   else
   {
     PyErr_SetString(PyExc_ValueError, "Expecting an integer");
     return nullptr;
   }
 }
 
+%typemap(in) lldb::StateType {
+  using namespace lldb_private;
+  if (PythonInteger::Check($input))
+  {
+    PythonInteger py_int(PyRefType::Borrowed, $input);
+    int64_t state_type_value = py_int.GetInteger() ;
+
+    if (state_type_value > lldb::StateType::kLastStateType) {
+      PyErr_SetString(PyExc_ValueError, "Not a valid StateType value");
+      return nullptr;
+    }
+    $1 = static_cast<lldb::StateType>(state_type_value);
+  }
+  else
+  {
+    PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+    return nullptr;
+  }
+}
+
 /* Typemap definitions to allow SWIG to properly handle char buffer. */
 
 // typemap for a char buffer
 // See also SBThread::GetStopDescription.
 %typemap(in) (char *dst, size_t dst_len) {
    if (!PyInt_Check($input)) {
        PyErr_SetString(PyExc_ValueError, "Expecting an integer");
        return NULL;

lldb/trunk/unittests/Utility/StateTest.cpp

 #include "lldb/Utility/State.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "gtest/gtest.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 TEST(StateTest, Formatv) {
-  EXPECT_EQ("exited", llvm::formatv("{0}", eStateExited).str());
+  EXPECT_EQ("invalid", llvm::formatv("{0}", eStateInvalid).str());
+  EXPECT_EQ("unloaded", llvm::formatv("{0}", eStateUnloaded).str());
+  EXPECT_EQ("connected", llvm::formatv("{0}", eStateConnected).str());
+  EXPECT_EQ("attaching", llvm::formatv("{0}", eStateAttaching).str());
+  EXPECT_EQ("launching", llvm::formatv("{0}", eStateLaunching).str());
   EXPECT_EQ("stopped", llvm::formatv("{0}", eStateStopped).str());
-  EXPECT_EQ("unknown", llvm::formatv("{0}", StateType(-1)).str());
+  EXPECT_EQ("running", llvm::formatv("{0}", eStateRunning).str());
+  EXPECT_EQ("stepping", llvm::formatv("{0}", eStateStepping).str());
+  EXPECT_EQ("crashed", llvm::formatv("{0}", eStateCrashed).str());
+  EXPECT_EQ("detached", llvm::formatv("{0}", eStateDetached).str());
+  EXPECT_EQ("exited", llvm::formatv("{0}", eStateExited).str());
+  EXPECT_EQ("suspended", llvm::formatv("{0}", eStateSuspended).str());
+  
 }