[lldb] add printing of stdout compile errors to lldbsuite

[bbli]

Summary

This patch will add printing of the output of stdout during compile errors, right below output of stderr.

[bbli]

Changed back to original function signature, as this method gets called once outside the class.

[labath]

lldb/packages/Python/lldbsuite/test_event/build_exception.py

7

This .decode call will be incorrect on python3 if lldb_extensions ever does not contain the stderr_content field. (That's because "<error output unavailable>", a string object, does not have a decode method -- only bytes objects can be decoded in python3.

By the looks of things, stderr_content will always be filled in, so you should be able to just replace this with direct member access (called_process_error.lldb_extensions.stderr_content.decode(...)).

Also, if I'm not mistaken, this will just concatenate stdout and stderr without any sort of delimiters, or caring about the order in which they are produced. That could end up being confusing because the compilers stderr will come before the compiler command which wrote it. If we're not going to be merging the two streams in the proper sequence (I described one way in https://reviews.llvm.org/D81697#2138665), then it would be better to prefix each stream with it's own header (like you've done in https://reviews.llvm.org/file/data/wxosaassniqt7keadc2v/PHID-FILE-6o2ea7gsgnrznam5k4ua/image.png), so that it is clear that one is looking at two independent streams.

lldb/packages/Python/lldbsuite/test_event/formatter/xunit.py

253–269 ↗

(On Diff #276590)

I believe this file is pretty much unused now. I've created D83545 to remove it.

[labath]

PS: Don't worry about the harbormaster failures too much. It's very picky these days...

[bbli]

Hi Pavel,

When I change the stderr argument to point to STDOUT inside the Popen function, which is inside the lldbtest.system function, the output actually gets mixed up. Whereas currently,, after running a couple times, the contents of stderr always gets printed out first and then stdout. In any case, should I just make two headers then?

[labath]

Using two headers is ok, though I'd like to know what you meant by the "mixed up" comment. Are you saying that the contents comes out nondeterministically? Can you provide an example of that?

[bbli]

Yeah so in this pic,

, you can see in the code that both stdout and stderr point to PIPE, and what gets printed out is the concatenation of the two. However, in

, where stderr now points to stdout, and I just print the stdout output, the "clang-11: error" messages from stderr gets mixed up with the results from stdout,

[labath]

In D83425#2151003, @bbli wrote:

Yeah so in this pic,

, you can see in the code that both stdout and stderr point to PIPE, and what gets printed out is the concatenation of the two. However, in

, where stderr now points to stdout, and I just print the stdout output, the "clang-11: error" messages from stderr gets mixed up with the results from stdout,

Yes, that's exactly what I wanted! :) It matches the output I would get when running that command in the terminal and it makes the error come out right after the compiler command which produced it (it more complex makefiles it may not be so clear which compiler command produced what error).

The fact that the first "random flag" error seemingly comes out before any compiler command is weird but not related to this problem. That happens because our makefile runs a silent (without echoing the command) compiler invocation to build the dependency file. We may possibly want to echo that too, but it doesn't matter that much -- in more realistic scenarios, the error will not be as simple as a missing flag, and the dependency command will succeed, but the error will come from the compilation step.

[bbli]

Modified stderr output instead of concatenating the stderr and stdout output strings.

[bbli]

Ok, I have revised the patch with the code from the first pic. I also moved the decode back to the format_build_error since it was there to begin with(not sure how much of a difference it makes). Also while we finalize this patch, is there another fix I can get started on?

[labath]

In D83425#2154593, @bbli wrote:

Ok, I have revised the patch with the code from the first pic. I also moved the decode back to the format_build_error since it was there to begin with(not sure how much of a difference it makes). Also while we finalize this patch, is there another fix I can get started on?

Sure. I don't want to bore you with something too menial. How big of a project would you be interested in? One thing that would be very helpful is to come up with some kind of a strategy for testing the "gui" mode of lldb, as we currently have no good way of testing that. I've described one idea in https://reviews.llvm.org/D82522#2113608, but I'm open to other options too....

lldb/packages/Python/lldbsuite/test/lldbtest.py

443

Setting this no longer makes sense, as it will always be empty. Please remove that. Maybe also rename stdout_content to indicate it also contains stderr. Just plain "output" might suffice?

448

this error result also doesn't make sense. It looks like you'll also need to update the usage in getCompilerVersion (line 1276).

[bbli]

Changed getCompilerVersion and a function in TestDataFormatterSkipSummary to account for stderr being merged with stdout now

[bbli]

Setting this no longer makes sense, as it will always be empty. Please remove that. Maybe also rename stdout_content to indicate it also contains stderr. Just plain "output" might suffice?

Ok, deleted it and changed "stdout_content" to "combined_content".

this error result also doesn't make sense. It looks like you'll also need to update the usage in getCompilerVersion

Ohh your right. I should have check whether other functions depended on stderr stream. So checking the rest of the codebase, it seems the output of system is also used in TestDataFormatterSkipSummary.py, but b/c it only triggers on Macs, I didn't see a failure when I made the change. I have now modified both files accordingly.

[labath]

In D83425#2161255, @bbli wrote:

Setting this no longer makes sense, as it will always be empty. Please remove that. Maybe also rename stdout_content to indicate it also contains stderr. Just plain "output" might suffice?

Ok, deleted it and changed "stdout_content" to "combined_content".

Thanks this looks good now. I presume you don't have commit access and need someone to commit this for you?

this error result also doesn't make sense. It looks like you'll also need to update the usage in getCompilerVersion

Ohh your right. I should have check whether other functions depended on stderr stream. So checking the rest of the codebase, it seems the output of system is also used in TestDataFormatterSkipSummary.py, but b/c it only triggers on Macs, I didn't see a failure when I made the change.

LOL, I wonder when was the last time anyone ran the lldb test suite against gcc on a mac. @aprantl, can we just delete the version check blurb in TestDataFormatterSkipSummary.py?

[bbli]

I presume you don't have commit access and need someone to commit this for you?

Yup, that would be great!

lldb/packages/Python/lldbsuite/test/lldbtest.py

for shellCommand in commandList:
         if 'stdout' in kwargs:
             raise ValueError(
                 'stdout argument not allowed, it will be overridden.')
         if 'shell' in kwargs and kwargs['shell'] == False:
             raise ValueError('shell=False not allowed')
         process = Popen(
             shellCommand,
             stdout=PIPE,
-            stderr=PIPE,
+            stderr=STDOUT,
             shell=True,
             **kwargs)
         pid = process.pid
         this_output, this_error = process.communicate()
         retcode = process.poll()
 
         if retcode:
             cmd = kwargs.get("args")
             if cmd is None:
                 cmd = shellCommand
             cpe = CalledProcessError(retcode, cmd)
             # Ensure caller can access the stdout/stderr.
             cpe.lldb_extensions = {
-                "stdout_content": this_output,
-                "stderr_content": this_error,
+                "combined_output": this_output,
                 "command": shellCommand
             }
             raise cpe
         output = output + this_output.decode("utf-8")
-        error = error + this_error.decode("utf-8")
-    return (output, error)
+    return output
 
 
 def getsource_if_available(obj):
     """
     Return the text of the source code for an object if available.  Otherwise,
     a print representation is returned.
     """
     import inspect
class Base(unittest2.TestCase):
 
     def getCompilerVersion(self):
         """ Returns a string that represents the compiler version.
             Supports: llvm, clang.
         """
         version = 'unknown'
 
         compiler = self.getCompilerBinary()
-        version_output = system([[compiler, "-v"]])[1]
+        version_output = system([[compiler, "-v"]])
         for line in version_output.split(os.linesep):
             m = re.search('version ([0-9\.]+)', line)
             if m:
                 version = m.group(1)
         return version
 
     def getDwarfVersion(self):
         """ Returns the dwarf version generated by clang or '0'. """

lldb/packages/Python/lldbsuite/test_event/build_exception.py

 class BuildError(Exception):
 
     def __init__(self, called_process_error):
         super(BuildError, self).__init__("Error when building test subject")
         self.command = called_process_error.lldb_extensions.get(
             "command", "<command unavailable>")
-        self.build_error = called_process_error.lldb_extensions.get(
-            "stderr_content", "<error output unavailable>")
+        self.build_error = called_process_error.lldb_extensions["combined_output"]
 
     def __str__(self):
         return self.format_build_error(self.command, self.build_error)
 
     @staticmethod
     def format_build_error(command, command_output):
         return "Error when building test subject.\n\nBuild Command:\n{}\n\nBuild Command Output:\n{}".format(
             command, command_output.decode("utf-8"))

lldb/test/API/functionalities/data-formatter/data-formatter-skip-summary/TestDataFormatterSkipSummary.py

def data_formatter_commands(self):
                 'm_child2 = Level 4',
                 '}'])
 
         # Bad debugging info on SnowLeopard gcc (Apple Inc. build 5666).
         # Skip the following tests if the condition is met.
         if self.getCompiler().endswith('gcc') and not self.getCompiler().endswith('llvm-gcc'):
             import re
             gcc_version_output = system(
-                [[lldbutil.which(self.getCompiler()), "-v"]])[1]
+                [[lldbutil.which(self.getCompiler()), "-v"]])
             self.trace("my output:", gcc_version_output)
             for line in gcc_version_output.split(os.linesep):
                 m = re.search('\(Apple Inc\. build ([0-9]+)\)', line)
                 self.trace("line:", line)
                 if m:
                     gcc_build = int(m.group(1))
                     self.trace("gcc build:", gcc_build)
                     if gcc_build >= 5666: