Hide runtime support values such as clang's __vla_expr from frame variable

[aprantl]

Summary

by respecting the "artificial" attribute on variables. Function arguments that are artificial and useful to end-users are being whitelisted by the language runtime.

[jingham]

Could you add a comment (probably best in LanguageRuntime.h) saying what a RuntimeSupportValue is?

Also, so far as I can tell, this patch works because we use the CPP language runtime for C++ methods is an ObjC++ file, and the ObjCLanguageRuntime for ObjC methods in ditto. That is surely the right thing to do, but it would be good to ensure that there's a test that asserts this behavior. I'm sure there must be but didn't see on a quick scan. Can you make sure there's such tests and if not add one?

[aprantl]

In D61451#1488186, @jingham wrote:

Could you add a comment (probably best in LanguageRuntime.h) saying what a RuntimeSupportValue is?

Done.

Also, so far as I can tell, this patch works because we use the CPP language runtime for C++ methods is an ObjC++ file, and the ObjCLanguageRuntime for ObjC methods in ditto. That is surely the right thing to do, but it would be good to ensure that there's a test that asserts this behavior. I'm sure there must be but didn't see on a quick scan. Can you make sure there's such tests and if not add one?

I'm not sure what exactly you want me to test there. Can you elaborate?

[aprantl]

Thanks you! There was indeed a bug that prevented us from recognizing the correct runtime for the C++ this pointer.

[jingham]

LGTM

[labath]

I think there is a problem with this patch. I don't believe the TestVLA part would work on any non-darwin platform (it doesn't on linux: http://lab.llvm.org:8014/builders/lldb-x86_64-debian/builds/923), and it works on darwin only accidentally.

The reason the fails on linux is that we don't have a "language runtime" plugin for the "C" language, so there is noone to ask if the variable is a runtime support value, and so we return false from ValueObject::IsRuntimeSupportValue. I believe the reason this works on darwin is because ValueObject::IsRuntimeSupportValue has a fallback (not sure why) which explicitly asks for the ObjC runtime if the desired language is not present. On linux, we don't have an ObjC runtime..

I can think of a couple of solutions here (adding a C language runtime, defaulting to return IsArtificial in ValueObject if no language runtime is present) but none of them seemed too obvious to commit straight-away, so I've disabled a part of that test for the time being (r359867). Nonetheless, could you please take a look at this problem?

[aprantl]

In D61451#1489109, @labath wrote:

I think there is a problem with this patch. I don't believe the TestVLA part would work on any non-darwin platform (it doesn't on linux: http://lab.llvm.org:8014/builders/lldb-x86_64-debian/builds/923), and it works on darwin only accidentally.

The reason the fails on linux is that we don't have a "language runtime" plugin for the "C" language, so there is noone to ask if the variable is a runtime support value, and so we return false from ValueObject::IsRuntimeSupportValue. I believe the reason this works on darwin is because ValueObject::IsRuntimeSupportValue has a fallback (not sure why) which explicitly asks for the ObjC runtime if the desired language is not present. On linux, we don't have an ObjC runtime..

I can think of a couple of solutions here (adding a C language runtime, defaulting to return IsArtificial in ValueObject if no language runtime is present) but none of them seemed too obvious to commit straight-away, so I've disabled a part of that test for the time being (r359867). Nonetheless, could you please take a look at this problem?

Thanks for pointing this out. This should be addressed by r359925.

lldb/trunk/include/lldb/Target/CPPLanguageRuntime.h

public:
   /// \param[in] stop_others
   ///     True if other threads should pause during execution.
   ///
   /// \return
   ///      A ThreadPlan Shared pointer
   lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
                                                   bool stop_others);
 
+  bool IsRuntimeSupportValue(ValueObject &valobj) override;
 protected:
   // Classes that inherit from CPPLanguageRuntime can see and modify these
   CPPLanguageRuntime(Process *process);
 
 private:
   DISALLOW_COPY_AND_ASSIGN(CPPLanguageRuntime);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_CPPLanguageRuntime_h_

lldb/trunk/include/lldb/Target/LanguageRuntime.h

public:
 
   virtual lldb::SearchFilterSP CreateExceptionSearchFilter();
 
   virtual bool GetTypeBitSize(const CompilerType &compiler_type,
                               uint64_t &size) {
     return false;
   }
 
+  /// Identify whether a value is a language implementation detaul
+  /// that should be hidden from the user interface by default.
   virtual bool IsRuntimeSupportValue(ValueObject &valobj) { return false; }
 
   virtual void ModulesDidLoad(const ModuleList &module_list) {}
 
   // Called by the Clang expression evaluation engine to allow runtimes to
   // alter the set of target options provided to the compiler. If the options
   // prototype is modified, runtimes must return true, false otherwise.
   virtual bool GetOverrideExprOptions(clang::TargetOptions &prototype) {

lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h

public:
 
   virtual void SymbolsDidLoad(const ModuleList &module_list) {
     m_negative_complete_class_cache.clear();
   }
 
   bool GetTypeBitSize(const CompilerType &compiler_type,
                       uint64_t &size) override;
 
+  /// Check whether the name is "self" or "_cmd" and should show up in
+  /// "frame variable".
+  static bool IsWhitelistedRuntimeValue(ConstString name);
+  bool IsRuntimeSupportValue(ValueObject &valobj) override;
+
 protected:
   // Classes that inherit from ObjCLanguageRuntime can see and modify these
   ObjCLanguageRuntime(Process *process);
 
   virtual bool CalculateHasNewLiteralsAndIndexing() { return false; }
 
   bool ISAIsCached(ObjCISA isa) const {
     return m_isa_to_descriptor.find(isa) != m_isa_to_descriptor.end();

lldb/trunk/packages/Python/lldbsuite/test/lang/c/vla/TestVLA.py

 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import decorators
 import lldbsuite.test.lldbutil as lldbutil
 
 
 class TestVLA(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @decorators.skipIf(compiler="clang", compiler_version=['<', '8.0'])
     def test_vla(self):
         self.build()
         _, process, _, _ = lldbutil.run_to_source_breakpoint(
             self, "break here", lldb.SBFileSpec('main.c'))
 
+        # Make sure no helper expressions show up in frame variable.
+        var_opts = lldb.SBVariablesOptions()
+        var_opts.SetIncludeArguments(False)
+        var_opts.SetIncludeLocals(True)
+        var_opts.SetInScopeOnly(True)
+        var_opts.SetIncludeStatics(False)
+        var_opts.SetIncludeRuntimeSupportValues(False)
+        var_opts.SetUseDynamic(lldb.eDynamicCanRunTarget)
+        all_locals = self.frame().GetVariables(var_opts)
+        self.assertEqual(len(all_locals), 1)
+
         def test(a, array):
             for i in range(a):
                 self.expect("fr v vla[%d]"%i, substrs=["int", "%d"%(a-i)])
                 self.expect("expr vla[%d]"%i, substrs=["int", "%d"%(a-i)])
-            self.expect("frame var vla", substrs=array)
-            self.expect("expr      vla", error=True, substrs=["incomplete"])
+            self.expect("fr v vla", substrs=array)
+            self.expect("expr vla", error=True, substrs=["incomplete"])
 
         test(2, ["int []", "[0] = 2, [1] = 1"])
         process.Continue()
         test(4, ["int []", "[0] = 4, [1] = 3, [2] = 2, [3] = 1"])
 

lldb/trunk/packages/Python/lldbsuite/test/lang/objcxx/hide-runtime-values/Makefile

+LEVEL = ../../../make
+
+OBJCXX_SOURCES := main.mm
+LDFLAGS = $(CFLAGS) -lobjc -framework Foundation
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/lang/objcxx/hide-runtime-values/TestObjCXXHideRuntimeValues.py

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+import os
+import unittest2
+
+
+class TestObjCXXHideRuntimeSupportValues(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    def test_hide_runtime_support_values(self):
+        self.build()
+        _, process, _, _ = lldbutil.run_to_source_breakpoint(
+            self, 'break here', lldb.SBFileSpec('main.mm'))
+
+        var_opts = lldb.SBVariablesOptions()
+        var_opts.SetIncludeArguments(True)
+        var_opts.SetIncludeLocals(True)
+        var_opts.SetInScopeOnly(True)
+        var_opts.SetIncludeStatics(False)
+        var_opts.SetIncludeRuntimeSupportValues(False)
+        var_opts.SetUseDynamic(lldb.eDynamicCanRunTarget)
+        values = self.frame().GetVariables(var_opts)
+
+        def shows_var(name):
+            for value in values:
+                if value.name == name:
+                    return True
+            return False
+        # ObjC method.
+        values = self.frame().GetVariables(var_opts)
+        self.assertFalse(shows_var("this"))
+        self.assertTrue(shows_var("self"))
+        self.assertTrue(shows_var("_cmd"))
+        self.assertTrue(shows_var("c"))
+
+        process.Continue()
+        # C++ method.
+        values = self.frame().GetVariables(var_opts)
+        self.assertTrue(shows_var("this"))
+        self.assertFalse(shows_var("self"))
+        self.assertFalse(shows_var("_cmd"))

lldb/trunk/packages/Python/lldbsuite/test/lang/objcxx/hide-runtime-values/main.mm

+#import <Foundation/Foundation.h>
+
+void baz() {}
+
+struct MyClass {
+  void bar() {
+    baz(); // break here
+  }
+};
+
+@interface MyObject : NSObject {}
+- (void)foo;
+@end
+
+@implementation MyObject
+- (void)foo {
+  MyClass c;
+  c.bar(); // break here
+}
+@end
+
+int main (int argc, char const *argv[]) {
+    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
+    id obj = [MyObject new];
+    [obj foo];
+    [pool release];
+    return 0;
+}

lldb/trunk/source/Symbol/ClangASTContext.cpp

ClangASTContext::GetMinimumLanguage(lldb::opaque_compiler_type_t type) {
   if (!type)
     return lldb::eLanguageTypeC;
 
   // If the type is a reference, then resolve it to what it refers to first:
   clang::QualType qual_type(GetCanonicalQualType(type).getNonReferenceType());
   if (qual_type->isAnyPointerType()) {
     if (qual_type->isObjCObjectPointerType())
       return lldb::eLanguageTypeObjC;
+    if (qual_type->getPointeeCXXRecordDecl())
+      return lldb::eLanguageTypeC_plus_plus;
 
     clang::QualType pointee_type(qual_type->getPointeeType());
-    if (pointee_type->getPointeeCXXRecordDecl() != nullptr)
+    if (pointee_type->getPointeeCXXRecordDecl())
       return lldb::eLanguageTypeC_plus_plus;
     if (pointee_type->isObjCObjectOrInterfaceType())
       return lldb::eLanguageTypeObjC;
     if (pointee_type->isObjCClassType())
       return lldb::eLanguageTypeObjC;
     if (pointee_type.getTypePtr() ==
         getASTContext()->ObjCBuiltinIdTy.getTypePtr())
       return lldb::eLanguageTypeObjC;

lldb/trunk/source/Target/CPPLanguageRuntime.cpp

 //===-- CPPLanguageRuntime.cpp
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/CPPLanguageRuntime.h"
+#include "lldb/Target/ObjCLanguageRuntime.h"
 
 #include <string.h>
 
 #include <memory>
 
 #include "llvm/ADT/StringRef.h"
 
 #include "lldb/Symbol/Block.h"
+#include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
 
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/UniqueCStringMap.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/ThreadPlanRunToAddress.h"
 #include "lldb/Target/ThreadPlanStepInRange.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
+static ConstString g_this = ConstString("this");
+
 // Destructor
 CPPLanguageRuntime::~CPPLanguageRuntime() {}
 
 CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
     : LanguageRuntime(process) {}
 
+bool CPPLanguageRuntime::IsRuntimeSupportValue(ValueObject &valobj) {
+  // All runtime support values have to be marked as artificial by the
+  // compiler. But not all artificial variables should be hidden from
+  // the user.
+  if (!valobj.GetVariable())
+    return false;
+  if (!valobj.GetVariable()->IsArtificial())
+    return false;
+
+  // Whitelist "this" and since there is no ObjC++ runtime, any ObjC names.
+  ConstString name = valobj.GetName();
+  if (name == g_this)
+    return false;
+  return !ObjCLanguageRuntime::IsWhitelistedRuntimeValue(name);
+}
+
 bool CPPLanguageRuntime::GetObjectDescription(Stream &str,
                                               ValueObject &object) {
   // C++ has no generic way to do this.
   return false;
 }
 
 bool CPPLanguageRuntime::GetObjectDescription(
     Stream &str, Value &value, ExecutionContextScope *exe_scope) {
if (!found_expected_start_string)
     return ret_plan_sp;
 
   AddressRange range_of_curr_func;
   sc.GetAddressRange(eSymbolContextEverything, 0, false, range_of_curr_func);
 
   StackFrameSP frame = thread.GetStackFrameAtIndex(0);
 
   if (frame) {
-    ValueObjectSP value_sp = frame->FindVariable(ConstString("this"));
+    ValueObjectSP value_sp = frame->FindVariable(g_this);
 
     CPPLanguageRuntime::LibCppStdFunctionCallableInfo callable_info =
         FindLibCppStdFunctionCallableInfo(value_sp);
 
     if (callable_info.callable_case != LibCppStdFunctionCallableCase::Invalid &&
         value_sp->GetValueIsValid()) {
       // We found the std::function wrapped callable and we have its address.
       // We now create a ThreadPlan to run to the callable.

lldb/trunk/source/Target/ObjCLanguageRuntime.cpp

 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Symbol/TypeList.h"
+#include "lldb/Symbol/Variable.h"
 #include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timer.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DJB.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // Destructor
 ObjCLanguageRuntime::~ObjCLanguageRuntime() {}
 
 ObjCLanguageRuntime::ObjCLanguageRuntime(Process *process)
     : LanguageRuntime(process), m_impl_cache(),
       m_has_new_literals_and_indexing(eLazyBoolCalculate),
       m_isa_to_descriptor(), m_hash_to_isa_map(), m_type_size_cache(),
       m_isa_to_descriptor_stop_id(UINT32_MAX), m_complete_class_cache(),
       m_negative_complete_class_cache() {}
 
+bool ObjCLanguageRuntime::IsWhitelistedRuntimeValue(ConstString name) {
+  static ConstString g_self = ConstString("self");
+  static ConstString g_cmd = ConstString("_cmd");
+  return name == g_self || name == g_cmd;
+}
+
+bool ObjCLanguageRuntime::IsRuntimeSupportValue(ValueObject &valobj) {
+  // All runtime support values have to be marked as artificial by the
+  // compiler. But not all artificial variables should be hidden from
+  // the user.
+  if (!valobj.GetVariable())
+    return false;
+  if (!valobj.GetVariable()->IsArtificial())
+    return false;
+
+  // Whitelist "self" and "_cmd".
+  return !IsWhitelistedRuntimeValue(valobj.GetName());
+}
+
 bool ObjCLanguageRuntime::AddClass(ObjCISA isa,
                                    const ClassDescriptorSP &descriptor_sp,
                                    const char *class_name) {
   if (isa != 0) {
     m_isa_to_descriptor[isa] = descriptor_sp;
     // class_name is assumed to be valid
     m_hash_to_isa_map.insert(std::make_pair(llvm::djbHash(class_name), isa));
     return true;