[lldb] Set result error state in 'frame variable'

[kastiglione]

Summary

Ensure that errors in frame variable are reflected in result object.

The statistics for frame variable show invocations as being successful, even
when executing one of the error paths.

This change replaces result.GetErrorStream() with result.AppendError(),
which also sets the status to eReturnStatusFailed.

[kastiglione]

s/warning/error

[kastiglione]

lldb/source/Commands/CommandObjectFrame.cpp

560–564

this regex error is a weird edge case. For example, considering running:

frame var --regex matchesSomeVars doesntMatchAnyVars

if the doesntMatchAnyVars pattern has no matches, then the command prints an error, and the result would be marked as an error. But if the matchesSomeVars does have matches, then we have a partial success / partial failure. In such a case, should the result be marked success, or failure? I don't know, but I would lean to success since it does entirely fail. Maybe a user could expect some patterns to match and some to not match. For example: a user alias that prints any variables based on a set of patterns they're interested in.

[kastiglione]

lldb/source/Commands/CommandObjectFrame.cpp

560–564

I think this could be changed to a warning. @jingham what do you think?

[JDevlieghere]

lldb/source/Commands/CommandObjectFrame.cpp

560–564

Sounds reasonable to me

[clayborg]

I would vote that if any argument or option is specified, and results in something not being found, that an error be returned. The error state doesn't do anything for the user on the command line, but it is nice to know that something failed to produce a result due to arguments being given. Any tests that are failing now when they didn't before can be modified.

Slo the cases I can think of are:

• "frame variable" with no args that results in no variables being displayed because there is no debug info returns an error like "no debug information for current stack frame"
• "frame variable" with no args that results in no variables being displayed and there is debug info, returns an error like "no variables found in debug information"
• "frame variable" with any args or options that do not find a match returns an error with possible multiple strings like "no variables found that match the name '%s'" (for a argument) or "no variables found that matches the regular expression '%s'" (for options)

[kastiglione]

In D116788#3231612, @clayborg wrote:

I would vote that if any argument or option is specified, and results in something not being found, that an error be returned.

Is this in response to my question about regexes? It seems to be. Specifically, you're of the opinion that any failing regex results in an error, even when other regexes do match? Your point about it not affecting the command line is a good point, thanks.

All your other points sound good to me too.

Any tests that are failing now when they didn't before can be modified.

Yes there were a couple, D116901 and D116863.

[clayborg]

In D116788#3232105, @kastiglione wrote:

In D116788#3231612, @clayborg wrote:

I would vote that if any argument or option is specified, and results in something not being found, that an error be returned.

Is this in response to my question about regexes? It seems to be. Specifically, you're of the opinion that any failing regex results in an error, even when other regexes do match? Your point about it not affecting the command line is a good point, thanks.

It is about anything that can't be properly displayed when asked. So lets pretend we have only two variables available "a" and "b". Any of these should return an error:

(lldb) frame variable bad // No variable named bad, so return an error like "no variables found that match 'bad'"
(lldb) frame variable a bad // No variable named bad, so return an error like "no variables found that match 'bad'" though we would display the value for "a" followed by the error message
(lldb) frame variable a bad b // No variable named bad, so return an error like "no variables found that match 'bad'" though we would display the value for "a" followed by the error message and followed by "b"'s value
(lldb) frame variable --regex b bad // regex "b" will match, but not regex "bad", so an error like "no matches found for regex 'bad'" should be returned
(lldb) frame variable --regex bad worse // neither regex will match, so an error like "no matches found for regex 'bad' and 'worse'" should be returned

If the user really wants to find either "a" or "bad", they can modify their regex so that it succeeds when it can since it would match a or bad:

(lldb) frame variable --regex "a|bad"

Or they can do separate invocations to ensure it succeeds or fails correctly.

So any argument that is specified that has no matches should produce an error. I am open to suggestions here, as these are just my initial thoughts.

All your other points sound good to me too.

Any tests that are failing now when they didn't before can be modified.

Yes there were a couple, D116901 and D116863.

Nice, those should be easy to fix.

Let me know your thoughts as mine are just my initial thoughts after thinking about things a bit more.

[kastiglione]

Let me know your thoughts as mine are just my initial thoughts after thinking about things a bit more.

For most of what you said, I was thinking the same. The only place where I wasn't as sure about (regex), I was on the fence. Your cases and arguments all seem good to me. I think the consistency is also good.

thanks

[jingham]

I agree that if we are going to start putting in errors for not finding listed arguments to “frame var” we should do it consistently, and Greg’s list seems good - along with the error “No debug info” when “frame var” is run in a frame w/o debug info or recognizers.

The one thing that bothers me slightly about this is that if I run “frame var” from HandleCommand in a script, with this change I can’t rely on the command status to tell whether the command output had anything interesting in it. That seems awkward.

NB: speaking just for myself, I’m actually a little in favor of that potential breakage, since “the problem just shows that you shouldn’t be using HandleCommand in scripts”. But my non-mean self did feel like it ought to raise the issue…

Jim

[kastiglione]

if I run “frame var” from HandleCommand in a script, with this change I can’t rely on the command status to tell whether the command output had anything interesting in it. That seems awkward.

This could be resolved by introducing a new enum value to indicate partial success, partial failure. I think that would be reasonable. I don't think that should block this change though, do you?

[jingham]

Indeed not. Since you can get everything you would need straightforwardly using SBFrame::FindVariables, there’s no pressing need to give another way to do it using HandleCommand.

Jim

lldb/source/Commands/CommandObjectFrame.cpp

 //===-- CommandObjectFrame.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 "CommandObjectFrame.h"
if (variable_list) {
                         if (var_sp->DumpDeclaration(&s, show_fullpaths,
                                                     show_module))
                           s.PutCString(": ");
                       }
                       valobj_sp->Dump(result.GetOutputStream(), options);
                     }
                   }
                 }
               } else if (num_matches == 0) {
-                result.GetErrorStream().Printf("error: no variables matched "
-                                               "the regular expression '%s'.\n",
-                                               entry.c_str());
+                result.AppendWarningWithFormat(
+                    "no variables matched the regular expression '%s'.",
               }
             } else {
               if (llvm::Error err = regex.GetError())
-                result.GetErrorStream().Printf(
-                    "error: %s\n", llvm::toString(std::move(err)).c_str());
+                result.AppendError(llvm::toString(std::move(err)));
               else
-                result.GetErrorStream().Printf(
-                    "error: unknown regex error when compiling '%s'\n",
-                    entry.c_str());
+                result.AppendErrorWithFormat(
+                    "unknown regex error when compiling '%s'", entry.c_str());
             }
           } else // No regex, either exact variable names or variable
                  // expressions.
           {
             Status error;
             uint32_t expr_path_options =
                 StackFrame::eExpressionPathOptionCheckPtrVsMember |
                 StackFrame::eExpressionPathOptionsAllowDirectIVarAccess |
if (variable_list) {
               options.SetVariableFormatDisplayLanguage(
                   valobj_sp->GetPreferredDisplayLanguage());
 
               Stream &output_stream = result.GetOutputStream();
               options.SetRootValueObjectName(
                   valobj_sp->GetParent() ? entry.c_str() : nullptr);
               valobj_sp->Dump(output_stream, options);
             } else {
-              const char *error_cstr = error.AsCString(nullptr);
-              if (error_cstr)
-                result.GetErrorStream().Printf("error: %s\n", error_cstr);
+              if (auto error_cstr = error.AsCString(nullptr))
+                result.AppendError(error_cstr);
               else
-                result.GetErrorStream().Printf("error: unable to find any "
-                                               "variable expression path that "
-                                               "matches '%s'.\n",
-                                               entry.c_str());
+                result.AppendErrorWithFormat(
+                    "unable to find any variable expression path that matches "
+                    "'%s'.",
             }
           }
         }
       } else // No command arg specified.  Use variable_list, instead.
       {
         const size_t num_variables = variable_list->GetSize();
         if (num_variables > 0) {
           for (size_t i = 0; i < num_variables; i++) {
if (variable_list) {
                 options.SetRootValueObjectName(
                     var_sp ? var_sp->GetName().AsCString() : nullptr);
                 valobj_sp->Dump(result.GetOutputStream(), options);
               }
             }
           }
         }
       }
+      if (result.GetStatus() != eReturnStatusFailed)
         result.SetStatus(eReturnStatusSuccessFinishResult);
     }
 
     if (m_option_variable.show_recognized_args) {
       auto recognized_frame = frame->GetRecognizedFrame();
       if (recognized_frame) {
         ValueObjectListSP recognized_arg_list =
             recognized_frame->GetRecognizedArguments();
         if (recognized_arg_list) {