[llvm][utils] Fix llvm::Optional summary provider

[kastiglione]

Summary

Returning None from a Python type summary results in a summary string of "None". To indicate no summary string, the implementation should return the empty string.

This change fixes a bug where the summary provider for llvm::Optional would incorrectly show None. This would happen when the underlying type itself had no summary provider. Now, when the underlying type has a summary string, that string is used, but when there is no summary, then the Optional will also have no summary (return '').

[shafik]

Do we have any tests for these?

[aprantl]

We don't at the moment. We could add a test to the LLDB testsuite that compiles a test program against Support. There's some CMake trickery necessary for this, which is probably why we've shied away from it thus far.

[kastiglione]

There's some CMake trickery

Can you shed some more light on this? Am I understanding right: these formatters are llvm, but the test would be in lldb? It seems weird that something in llvm/ would only have tests in lldb/. Would it be bad to move this file into lldb, and then we can test there? With llvm being a monorepo, a source checkout should have access at any path. Maybe the issue would be llvm installations and packages that don't include lldb too?

[aprantl]

In D122041#3405821, @kastiglione wrote:

There's some CMake trickery

Can you shed some more light on this? Am I understanding right: these formatters are llvm, but the test would be in lldb? It seems weird that something in llvm/ would only have tests in lldb/.

You need an LLDB in order to test the dataformatter, that's why I thought the test makes most sense there. The other natural place would be cross-project-tests/.

Would it be bad to move this file into lldb, and then we can test there?

It's nice & consistent to have the LLDB data formatters next to the GDB dataformatters, but I don't very strong feelings about this.

With llvm being a monorepo, a source checkout should have access at any path. Maybe the issue would be llvm installations and packages that don't include lldb too?

The test would need to build a debuginfo-enabled binary that links against Support (which, I now realize, does not need debug info), So maybe this is not going to be much more complicated than any unit tests we are building.

llvm/utils/lldbDataFormatters.py

if hasVal == 0:
         return None
 
     underlying_type = storage.GetType().GetTemplateArgumentType(0)
     storage = storage.GetChildMemberWithName('value')
     return storage.Cast(underlying_type)
 
 def OptionalSummaryProvider(valobj, internal_dict):
     val = GetOptionalValue(valobj)
-    return val.summary if val else 'None'
+    if val is None:
+        return 'None'
+    if val.summary:
+        return val.summary
+    return ''
 
 class OptionalSynthProvider:
     """Provides deref support to llvm::Optional<T>"""
     def __init__(self, valobj, internal_dict):
         self.valobj = valobj
 
     def num_children(self):
         return self.valobj.num_children