[lldb/DataFormatters] Display null C++ pointers as nullptr

[JDevlieghere]

Summary

The original motivation for this patch was to display a null
std::string pointer as nullptr instead of "", but the fix
seemed generic enough to be done for all C++ summary providers.

[friss]

I haven't tested the libstdc++ part, would be great if someone could confirm this works.

[shafik]

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/string/main.cpp

13 ↗

(On Diff #253927)

How about string_ptr_null

[labath]

In D77153#1952952, @friss wrote:

I haven't tested the libstdc++ part, would be great if someone could confirm this works.

This doesn't work work for libstdc++ (I get <parent failed to evaluate: parent is NULL>), presumably because the code uses the simple StringSummaryFormat (new StringSummaryFormat(stl_summary_flags, "${var._M_dataplus._M_p}")) instead of CXXFunctionSummaryFormat However, it doesn't make the situation any worse either, so you can just drop the libstdc++ test changes and carry on.

[labath]

It might be worth mentioning though that the title of the patch is somewhat misleading -- I believe the CXX in CXXFunctionSummaryFormat refers to the language the formatter is implemented in, not the language of the type itself. So this patch affects all types whose formatters are implemented as c++ functions, not all c++ pointers in the target program.

[friss]

In D77153#1954154, @labath wrote:

It might be worth mentioning though that the title of the patch is somewhat misleading -- I believe the CXX in CXXFunctionSummaryFormat refers to the language the formatter is implemented in, not the language of the type itself. So this patch affects all types whose formatters are implemented as c++ functions, not all c++ pointers in the target program.

Wow, thanks for pointing this out. It should have clicked when I saw ScriptSummaryFormat besides it, but I was indeed completely confused. Let me rework this

[friss]

I was checking whether there is a way to catch null pointer before a type summary is even chosen. And found out that such logic exists, but only for Objective-C. Languages have a IsNilReference() virtual method that can tell whether a ValueObject is a nil reference. It's only implemented for ObjectiveC and value objects that test positive for this method are displayed as "nil" is the generic code of ValueObjectPrinter.cpp. I can see a few options:

• I can rework the patch to affect only the libc++ string formatter which was the issue I was trying to solve in the first place
• I can implement IsNilReference() for C++ and change ValueObjectPrinter to print C++ "nil references" as "nullptr".
• I do the above and add another customization point in the Language plugins which tells ValueObjectPrinter out to display "nil" references, so that there's no Language specific code in ValueObjectPrinter.

A few additional questions come to mind though:

• Was the intent if IsNilReference() to be Objective-C specific in the first place?
• Should we do the same thing for C and NULL?
• C, C++ and Objective-C(++) being highly intermingled, would something like this even do the right thing in all cases?

[labath]

In D77153#1955829, @friss wrote:

I was checking whether there is a way to catch null pointer before a type summary is even chosen. And found out that such logic exists, but only for Objective-C. Languages have a IsNilReference() virtual method that can tell whether a ValueObject is a nil reference. It's only implemented for ObjectiveC and value objects that test positive for this method are displayed as "nil" is the generic code of ValueObjectPrinter.cpp. I can see a few options:

• I can rework the patch to affect only the libc++ string formatter which was the issue I was trying to solve in the first place
• I can implement IsNilReference() for C++ and change ValueObjectPrinter to print C++ "nil references" as "nullptr".
• I do the above and add another customization point in the Language plugins which tells ValueObjectPrinter out to display "nil" references, so that there's no Language specific code in ValueObjectPrinter.

All of those options sound fine to me. Making that generic is obviously preferable, but I don't know how much work that would be.

The thing that I found surprising when looking at this is that std::string has the exact same summary as a std::string * which points to it. I would have expected at least some subtle hint (perhaps by prepending the summary with a hex pointer value?) to indicate that we're looking at a pointer. This would be doable if the generic code had some way to understand pointers (and their nullness).

A few additional questions come to mind though:

• Was the intent if IsNilReference() to be Objective-C specific in the first place?
• Should we do the same thing for C and NULL?
• C, C++ and Objective-C(++) being highly intermingled, would something like this even do the right thing in all cases?

Good questions, and I don't really have an answer for that. I don't think that IsNilReference should be c++-specific. Printing C pointers as NULL would be nice, but i don't know if we have any summaries for C types anyway.

[friss]

Implement null C++ printing at the ValueObjectPrinter level

[friss]

Remove the now useless code in TypeSummary.cpp, and change the C++
implementation of IsNilReference() to return true soemtimes. This
breaks other tests though...

[friss]

This looks like what I'd like to implement, but unfortunately it breaks other tests. Some C tests start printing null pointers as nullptr too. I suppose this is because the expression evaluator is always in C++ mode. Is there a way to get the origin type/language of a variable through a ValueObject?

[jingham]

ValueObjects know their execution context, and so they can get to their frame, if they have one. The language of the frame seems like the best thing to clue off here.

[JDevlieghere]

• Add fallback for finding the language
• Fix tests

[shafik]

So this work for primitive types like int* etc?

[JDevlieghere]

In D77153#2386740, @shafik wrote:

So this work for primitive types like int* etc?

No, Jim probably know the details, but this only applies to composite types.

[JDevlieghere]

Fix the C test

[JDevlieghere]

lldb/source/DataFormatters/ValueObjectPrinter.cpp

372

If we had a C runtime we could print "NULL", but I don't think it's worth adding that just for this. Another alternative would be to just have a switch based on the LanguageType, but that seems like a pretty bad idea from a layering perspective.

[teemperor]

A few comments, but otherwise this seems good.

lldb/include/lldb/Target/Language.h

218

/// Returns the summary string that should be used for a ValueObject for which IsNilReference() is true or something like that.

Also I think this probably should have a Get prefix like the other methods in this class.

lldb/source/DataFormatters/ValueObjectPrinter.cpp

372

Not sure what to think about hardcoding 0x0 here. You can specify all kind of formatters that would format the value here differently and this seems kinda inconsistent.

(lldb) expr --format d -- nullptr
(nullptr_t) $2 = 0
(lldb) expr --format b -- nullptr
(nullptr_t) $3 = 0b0000000000000000000000000000000000000000000000000000000000000000

Could we just fall to the default formatter if we don't have a special summary from the plugin?

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

1138

Nit: !Language::LanguageIsCPlusPlus(valobj.GetCompilerType().GetMinimumLanguage()) ? (as we might return a specific C++ standard in the future from GetMinimumLanguage).

[jingham]

It seems weird that even if you had a summary formatter for some pointer type that was trying to print "DANGER WILL ROBINSON" when the pointer value was 0x0, we will override that and print "nullptr" in a C++ context or "nil" in an ObjC context. Seems like even if the value is Nil we should first consult the ValueObject's summary value and only do the nullptr/nil computation if it was empty.

But that bad behavior was already present for ObjC objects before this patch, you're just extending it to C++ or anything else that implements IsNilReference. So fixing that doesn't seem required for this patch.

lldb/source/DataFormatters/ValueObjectPrinter.cpp

390

If you wanted to you could check here if the languages hadn't assigned anything to Summary, and default to "NULL"? That would be consistent with our treating C as the fallback language rather than as a separate Language plugin. Not required...

lldb/include/lldb/Target/Language.h

 //===-- Language.h ---------------------------------------------------*- C++
 //-*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
public:
   virtual DumpValueObjectOptions::DeclPrintingHelper GetDeclPrintingHelper();
 
   virtual LazyBool IsLogicalTrue(ValueObject &valobj, Status &error);
 
   // for a ValueObject of some "reference type", if the value points to the
   // nil/null object, this method returns true
   virtual bool IsNilReference(ValueObject &valobj);
 
+  virtual llvm::StringRef NilReferenceSummaryString() { return {}; }
+
   // for a ValueObject of some "reference type", if the language provides a
   // technique to decide whether the reference has ever been assigned to some
   // object, this method will return true if such detection is possible, and if
   // the reference has never been assigned
   virtual bool IsUninitializedReference(ValueObject &valobj);
 
   virtual bool GetFunctionDisplayName(const SymbolContext *sc,
                                       const ExecutionContext *exe_ctx,

lldb/source/DataFormatters/ValueObjectPrinter.cpp

 //===-- ValueObjectPrinter.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
 //
 //===----------------------------------------------------------------------===//
 
else {
     if (val_cstr)
       value.assign(val_cstr);
   }
   const char *err_cstr = m_valobj->GetError().AsCString();
   if (err_cstr)
     error.assign(err_cstr);
 
   if (ShouldPrintValueObject()) {
-    if (IsNil())
-      summary.assign("nil");
-    else if (IsUninitialized())
+    if (IsNil()) {
+      Language *lang_plugin =
+          Language::FindPlugin(m_valobj->GetObjectRuntimeLanguage());
+      summary.assign(lang_plugin->NilReferenceSummaryString().str());
+    } else if (IsUninitialized())
       summary.assign("<uninitialized>");
     else if (m_options.m_omit_summary_depth == 0) {
       TypeSummaryImpl *entry = GetSummaryFormatter();
       if (entry)
         m_valobj->GetSummaryAsCString(entry, summary,
                                       m_options.m_varformat_language);
       else {
         const char *sum_cstr =
             m_valobj->GetSummaryAsCString(m_options.m_varformat_language);
         if (sum_cstr)
           summary.assign(sum_cstr);
       }
     }
   }
 }
 
 bool ValueObjectPrinter::PrintValueAndSummaryIfNeeded(bool &value_printed,
                                                       bool &summary_printed) {
   bool error_printed = false;
   if (ShouldPrintValueObject()) {
     if (!CheckScopeIfNeeded())
       m_error.assign("out of scope");
     if (m_error.empty()) {
       GetValueSummaryError(m_value, m_summary, m_error);
     }
     if (m_error.size()) {
       // we need to support scenarios in which it is actually fine for a value
       // to have no type but - on the other hand - if we get an error *AND*
       // have no type, we try to get out gracefully, since most often that
       // combination means "could not resolve a type" and the default failure

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

 //===-- CPlusPlusLanguage.h -------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
public:
   std::unique_ptr<TypeScavenger> GetTypeScavenger() override;
   lldb::TypeCategoryImplSP GetFormatters() override;
 
   HardcodedFormatters::HardcodedSummaryFinder GetHardcodedSummaries() override;
 
   HardcodedFormatters::HardcodedSyntheticFinder
   GetHardcodedSynthetics() override;
 
+  bool IsNilReference(ValueObject &valobj) override;
+
+  llvm::StringRef NilReferenceSummaryString() override { return "nullptr"; }
+
   bool IsSourceFile(llvm::StringRef file_path) const override;
 
   const Highlighter *GetHighlighter() const override { return &m_highlighter; }
 
   // Static Functions
   static void Initialize();
 
   static void Terminate();

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

 //===-- CPlusPlusLanguage.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
 //
 //===----------------------------------------------------------------------===//
 
g_formatters.push_back([](lldb_private::ValueObject &valobj,
       return nullptr;
     });
 
   });
 
   return g_formatters;
 }
 
+bool CPlusPlusLanguage::IsNilReference(ValueObject &valobj) {
+  if (valobj.GetCompilerType().GetMinimumLanguage() != eLanguageTypeC_plus_plus ||
+      !valobj.IsPointerType())
+    return false;
+  bool canReadValue = true;
+  bool isZero = valobj.GetValueAsUnsigned(0, &canReadValue) == 0;
+  return canReadValue && isZero;
+}
+
 bool CPlusPlusLanguage::IsSourceFile(llvm::StringRef file_path) const {
   const auto suffixes = {".cpp", ".cxx", ".c++", ".cc",  ".c",
                          ".h",   ".hh",  ".hpp", ".hxx", ".h++"};
   for (auto suffix : suffixes) {
     if (file_path.endswith_lower(suffix))
       return true;
   }
 
   // Check if we're in a STL path (where the files usually have no extension
   // that we could check for.
   return file_path.contains("/usr/include/c++/");
 }

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

 //===-- ObjCLanguage.h ------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
public:
   std::unique_ptr<TypeScavenger> GetTypeScavenger() override;
 
   bool GetFormatterPrefixSuffix(ValueObject &valobj, ConstString type_hint,
                                 std::string &prefix,
                                 std::string &suffix) override;
 
   bool IsNilReference(ValueObject &valobj) override;
 
+  llvm::StringRef NilReferenceSummaryString() override { return "nil"; }
+
   bool IsSourceFile(llvm::StringRef file_path) const override;
 
   const Highlighter *GetHighlighter() const override { return &m_highlighter; }
 
   // Static Functions
   static void Initialize();
 
   static void Terminate();

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py

def test_with_run_command(self):
                 '(%s::u16string) u16_string = u"ß水氶"'%ns,
                 # FIXME: This should have a 'u' prefix.
                 '(%s::u16string) u16_empty = ""'%ns,
                 '(%s::u32string) u32_string = U"🍄🍅🍆🍌"'%ns,
                 # FIXME: This should have a 'U' prefix.
                 '(%s::u32string) u32_empty = ""'%ns,
                 '(%s::basic_string<unsigned char, %s::char_traits<unsigned char>, '
                 '%s::allocator<unsigned char> >) uchar = "aaaaa"'%(ns,ns,ns),
+                '(%s::string *) null_str = nullptr'%ns,
         ])
 
         self.runCmd("n")
 
         TheVeryLongOne = self.frame().FindVariable("TheVeryLongOne")
         summaryOptions = lldb.SBTypeSummaryOptions()
         summaryOptions.SetCapping(lldb.eTypeSummaryUncapped)
         uncappedSummaryStream = lldb.SBStream()
def test_with_run_command(self):
                 '(%s::string) Q = "quite a long std::strin with lots of info inside it"'%ns,
                 '(%s::string) IHaveEmbeddedZeros = "a\\0b\\0c\\0d"'%ns,
                 '(%s::wstring) IHaveEmbeddedZerosToo = L"hello world!\\0てざ ル゜䋨ミ㠧槊 きゅへ狦穤襩 じゃ馩リョ 䤦監"'%ns,
                 '(%s::u16string) u16_string = u"ß水氶"'%ns,
                 '(%s::u32string) u32_string = U"🍄🍅🍆🍌"'%ns,
                 '(%s::u32string) u32_empty = ""'%ns,
                 '(%s::basic_string<unsigned char, %s::char_traits<unsigned char>, '
                 '%s::allocator<unsigned char> >) uchar = "aaaaa"'%(ns,ns,ns),
+                '(%s::string *) null_str = nullptr'%ns,
         ])
 
         # The test assumes that std::string is in its cap-size-data layout.
         is_alternate_layout = ('arm' in self.getArchitecture()) and self.platformIsDarwin()
         if is_64_bit and not is_alternate_layout:
             self.expect("frame variable garbage1", substrs=['garbage1 = Summary Unavailable'])
             self.expect("frame variable garbage2", substrs=['garbage2 = Summary Unavailable'])
             self.expect("frame variable garbage3", substrs=['garbage3 = Summary Unavailable'])
             self.expect("frame variable garbage4", substrs=['garbage4 = Summary Unavailable'])
             self.expect("frame variable garbage5", substrs=['garbage5 = Summary Unavailable'])

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp

 #include <string>
 #include <stdint.h>
 
 // For more information about libc++'s std::string ABI, see:
 //
 //   https://joellaity.com/2020/01/31/string.html
 
 // A corrupt string which hits the SSO code path, but has an invalid size.
int main()
     std::string TheVeryLongOne("1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890someText1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
     std::string IHaveEmbeddedZeros("a\0b\0c\0d",7);
     std::wstring IHaveEmbeddedZerosToo(L"hello world!\0てざ ル゜䋨ミ㠧槊 きゅへ狦穤襩 じゃ馩リョ 䤦監", 38);
     std::u16string u16_string(u"ß水氶");
     std::u16string u16_empty(u"");
     std::u32string u32_string(U"🍄🍅🍆🍌");
     std::u32string u32_empty(U"");
     std::basic_string<unsigned char> uchar(5, 'a');
+    std::string *null_str = nullptr;
 
 #if _LIBCPP_ABI_VERSION == 1
     std::string garbage1, garbage2, garbage3, garbage4, garbage5;
     if (sizeof(std::string) == sizeof(garbage_string_short_mode))
       memcpy((void *)&garbage1, &garbage_string_short_mode, sizeof(std::string));
     if (sizeof(std::string) == sizeof(garbage_string_long_mode1))
       memcpy((void *)&garbage2, &garbage_string_long_mode1, sizeof(std::string));
     if (sizeof(std::string) == sizeof(garbage_string_long_mode2))