[DebugInfo] Remove unnecessary flushes and add additional testing

[jhenderson]

Summary

The flushes previously existed to help ensure consistent error message output when stdout and stderr were passed to the same location. This is no longer necessary as errs() is now tied to outs().

Depends on D81156.

[dblaikie]

Not sure I feel great about having these things synchronized - seems likely there's lots of error messages that get interspersed with stdout in llvm-dwarfdump and we aren't likely to get all the stdout flushed appropriately, so maybe we shouldn't start/try? If the test is flakey/unreliable due to this - perhaps it should FileCheck stdout and stderr separately rather than piping them together?

(but I don't feel super strongly - I'm OK with this patch/direction if other folks think it's the right direction)

[JDevlieghere]

In D80803#2063224, @dblaikie wrote:

Not sure I feel great about having these things synchronized - seems likely there's lots of error messages that get interspersed with stdout in llvm-dwarfdump and we aren't likely to get all the stdout flushed appropriately, so maybe we shouldn't start/try? If the test is flakey/unreliable due to this - perhaps it should FileCheck stdout and stderr separately rather than piping them together?

+1. Given that the WarningHandler is configurable I don't think we should be making assumptions of what it's doing, even if printing stderr is the default. I would even go a step further and remove the existing flushes so the test don't rely on them.

[MaskRay]

I am in favor of synchronizing stdout/stderr, i.e. when redirected to a file, making the behavior similar to a pty (line buffered). This makes stderr output in tests more relevant, i.e. we usually do 2>&1 | FileCheck, now we can check stderr output is emitted in appropriate places. However, like other opinions, I am wary of littering flush all over the code base. Also note that flushing too frequently has a performance penalty when full buffering is otherwise used. (I once made llvm-objdump -d slow due to a change to make it line buffered)

Can we do something similar to D64165? Teach the warning/error (stderr) handler to flush stdout? If there is no stderr output, usual buffering (full buffering when the output is a file) is used. There is no performance penalty.

[dblaikie]

In D80803#2064936, @MaskRay wrote:

I am in favor of synchronizing stdout/stderr, i.e. when redirected to a file, making the behavior similar to a pty (line buffered). This makes stderr output in tests more relevant, i.e. we usually do 2>&1 | FileCheck, now we can check stderr output is emitted in appropriate places. However, like other opinions, I am wary of littering flush all over the code base. Also note that flushing too frequently has a performance penalty when full buffering is otherwise used. (I once made llvm-objdump -d slow due to a change to make it line buffered)

Can we do something similar to D64165? Teach the warning/error (stderr) handler to flush stdout? If there is no stderr output, usual buffering (full buffering when the output is a file) is used. There is no performance penalty.

If we're going to do it, yeah, D64165 (flushing outs() before the diagnostic, flushing the diagnostic stream after the diagnostic) would be the better direction. But even in that case - it seems it isn't consistently applied even within llvm-objdump.cpp (isn't done for command line warnings, nor for errors?)

[MaskRay]

In D80803#2065004, @dblaikie wrote:

In D80803#2064936, @MaskRay wrote:

I am in favor of synchronizing stdout/stderr, i.e. when redirected to a file, making the behavior similar to a pty (line buffered). This makes stderr output in tests more relevant, i.e. we usually do 2>&1 | FileCheck, now we can check stderr output is emitted in appropriate places. However, like other opinions, I am wary of littering flush all over the code base. Also note that flushing too frequently has a performance penalty when full buffering is otherwise used. (I once made llvm-objdump -d slow due to a change to make it line buffered)

Can we do something similar to D64165? Teach the warning/error (stderr) handler to flush stdout? If there is no stderr output, usual buffering (full buffering when the output is a file) is used. There is no performance penalty.

If we're going to do it, yeah, D64165 (flushing outs() before the diagnostic, flushing the diagnostic stream after the diagnostic) would be the better direction. But even in that case - it seems it isn't consistently applied even within llvm-objdump.cpp (isn't done for command line warnings, nor for errors?)

The idea is that some warning/error reporting facilities need outs().flush(). For llvm-objdump.cpp, reportWarning and reportError are the two utilities. It may be possible that some code path does not consistently flush outs(). We can fix them if someone observes problems.

[dblaikie]

In D80803#2065028, @MaskRay wrote:

In D80803#2065004, @dblaikie wrote:

In D80803#2064936, @MaskRay wrote:

I am in favor of synchronizing stdout/stderr, i.e. when redirected to a file, making the behavior similar to a pty (line buffered). This makes stderr output in tests more relevant, i.e. we usually do 2>&1 | FileCheck, now we can check stderr output is emitted in appropriate places. However, like other opinions, I am wary of littering flush all over the code base. Also note that flushing too frequently has a performance penalty when full buffering is otherwise used. (I once made llvm-objdump -d slow due to a change to make it line buffered)

Can we do something similar to D64165? Teach the warning/error (stderr) handler to flush stdout? If there is no stderr output, usual buffering (full buffering when the output is a file) is used. There is no performance penalty.

If we're going to do it, yeah, D64165 (flushing outs() before the diagnostic, flushing the diagnostic stream after the diagnostic) would be the better direction. But even in that case - it seems it isn't consistently applied even within llvm-objdump.cpp (isn't done for command line warnings, nor for errors?)

The idea is that some warning/error reporting facilities need outs().flush(). For llvm-objdump.cpp, reportWarning and reportError are the two utilities.

reportError doesn't seem to have the flushing (& also I'm not sure if it needs to use an extra raw_string_ostream - could write out to errs() directly, I think) & reportCmdLineWarning, reportCmdLineError (hmm, why are there different reportCmdLine* distinct from report*?)

It may be possible that some code path does not consistently flush outs(). We can fix them if someone observes problems.

Yeah, not the end of the world/fixing at least fewer places.

[MaskRay]

In D80803#2065086, @dblaikie wrote:

In D80803#2065028, @MaskRay wrote:

In D80803#2065004, @dblaikie wrote:

In D80803#2064936, @MaskRay wrote:

I am in favor of synchronizing stdout/stderr, i.e. when redirected to a file, making the behavior similar to a pty (line buffered). This makes stderr output in tests more relevant, i.e. we usually do 2>&1 | FileCheck, now we can check stderr output is emitted in appropriate places. However, like other opinions, I am wary of littering flush all over the code base. Also note that flushing too frequently has a performance penalty when full buffering is otherwise used. (I once made llvm-objdump -d slow due to a change to make it line buffered)

Can we do something similar to D64165? Teach the warning/error (stderr) handler to flush stdout? If there is no stderr output, usual buffering (full buffering when the output is a file) is used. There is no performance penalty.

If we're going to do it, yeah, D64165 (flushing outs() before the diagnostic, flushing the diagnostic stream after the diagnostic) would be the better direction. But even in that case - it seems it isn't consistently applied even within llvm-objdump.cpp (isn't done for command line warnings, nor for errors?)

The idea is that some warning/error reporting facilities need outs().flush(). For llvm-objdump.cpp, reportWarning and reportError are the two utilities.

reportError doesn't seem to have the flushing (& also I'm not sure if it needs to use an extra raw_string_ostream - could write out to errs() directly, I think) & reportCmdLineWarning, reportCmdLineError (hmm, why are there different reportCmdLine* distinct from report*?)

reportCmdLineWarning was introduced in D66418. It reports a warning not associated with a file. Many warnings/errors are associated with a file. error: filename: .... style output is convenient when a user greps a log file.

Good catch. If I add outs().flush() to reportError, no tests fail. We can add it when we see abnormal output. The logAllUnhandledErrors use can indeed be simplified.

It may be possible that some code path does not consistently flush outs(). We can fix them if someone observes problems.

Yeah, not the end of the world/fixing at least fewer places.

[jhenderson]

I found having the error messages in different places somewhat confusing. Worse still, it seemed like occasionally they weren't consistent between being run under FileCheck and when I ran llvm-dwarfdump directly, although that might have only been tangentially related to when things were flushed. I'll take a look at moving the flushing to the error handler. It makes sense not to penalise the normal use-case, i.e. where there are no errors, by flushing unnecessarily.

[jhenderson]

So moving the flushing to the warning handler is trickier than it first seems, because the default warning handler (which is the one used by llvm-dwarfdump) is a part of the WithColor interface now, and does not have access to the output stream used by the DWARFContext code, which may or may not be stdout. We could still go ahead and flush stdout in that function, as that will work for llvm-dwarfdump, but it's not guaranteed to achieve the intended effect in all cases. We could wrap the warning handler routine locally in the DWARFContext code to do the flushing, but it would prevent people using a non-flushing handler, which seems less than ideal.

[jhenderson]

Having investigated further, updating the defaultWarningHandler method to flush stdout does not solve the issue, because llvm-dwarfdump actually writes to a file stream, which means the thing being flushed is not the right stream. I think the right thing to do is to have llvm-dwarfdump provide its own warning and error handler to DWARFContext. That will allow it to flush the right stream at the right time.

[jhenderson]

Reimplemented flushing behaviour as part of the llvm-dwarfdump warning handler, as discussed. Also fixed an issue where the llvm-dwarfdump warning handler wasn't properly being copied around. I also have removed the two other existing flushes, as I believe we agree that it should be up to the handling methods to do the flushing as desired. Added @labath as a reviewer for this part, since he introduced them a few months ago.

[jhenderson]

See rGced45978a2a5b258421590214f46848e726144b8 for where the other two flushes were introduced. I think this fix is a better fix from llvm-dwarfdump's point of view, and other clients should be updated if and when necessary.

[JDevlieghere]

While I do prefer the current approach over the first revision of this path, I'm still not convinced that dwarfdump should be flushing. If you combine the streams you accept that there is no guaranteed order. Flushing is a band-aid and not a structural solution. This patch already shows that it doesn't scale. I don't think we should settle for "fix[ing] them if someone observes problems." I also don't see how that makes the errors more relevant in the tests. If they are not strict enough we should alter the error message instead of relying on the order.

[JDevlieghere]

In case it wasn't clear from my previous reply, I'm totally willing to be convinced that this is the right thing to do, I just hoping for a more compelling argument.

[dblaikie]

Looks good - I'm OK with this direction.

In D80803#2066021, @jhenderson wrote:

I found having the error messages in different places somewhat confusing. Worse still, it seemed like occasionally they weren't consistent between being run under FileCheck and when I ran llvm-dwarfdump directly, although that might have only been tangentially related to when things were flushed.

Yeah, it's certainly likely that sort of inconsistent behavior could come from flushing issues, especially under FileCheck - I'm kind of OK with that/think a possible "right thing" would be to ensure that we don't depend on the ordering between stdout and stderr. But, yeah, this approach of flushing around diagnostics seems sustainable enough that it's an OK alternative.

[dblaikie]

Going to revert my approval for now as @JDevlieghere 's concerns should be addressed/discussed (didn't mean to usurp his inquiry - just a race condition on feedback)

@JDevlieghere I think this does show a fairly scalable solution - limited to only a couple of diagnostic handlers.

But, yeah, I'm with you too - the error messages should be sufficiently self descriptive that they don't need to appear in particular places interleaved with stdout.

[MaskRay]

I also think this approach is scalable.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

57

Unneeded change after your other change landed?

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

506 ↗

(On Diff #267625)

errs().flush() can be deleted. stderr is not buffered. I requested this in the llvm-objdump patch when ychen added it, staging that there were two other instances. The two other instances seems to have gone, so I deleted errs().flush() as well yesterday.

If a user explicitly makes stderr buffered (via setvbuf), they should accept strangely interleaved streams.

[labath]

I think this is definitely better than having scattered flushes -- thanks for working on a more systemic solution.

I'm just going to throw one more idea out there. I came up with it when I was adding the original flushes. At the time I chose not to pursue it as I just wanted to get things done, but if we're going to discuss this problem in depth, I think it deserves consideration. If there's consensus that this is a good direction to go in, I'll be happy to create a patch for it

While I do prefer the current approach over the first revision of this path, I'm still not convinced that dwarfdump should be flushing. If you combine the streams you accept that there is no guaranteed order.

That is not really true. std::iostreams have the concept of "tied" streams, whose (I believe) sole purpose for existence is to ensure consistent ordering between cout and cerr streams. The way it works is that whenever a stream does a read or write operation from/to the backing storage, it first flushes the "tied" stream (if it has one). Normally, cin and cerr are tied to cout, and that's why patterns like cout << "Give me a number:" /* no newline*/; cin >> n; work. So, combining stdout and stderr is an issue that a lot of people considered important, and indeed there are a lot of tools out there which get it "right".

llvm streams have no "tied stream" concept, and I would stipulate that this is the real reason for why we're running into the problems here. The simplest and least obtrusive way this concept could be retrofitted into the existing llvm streams is to have a new class -- let's call it tied_raw_fd_ostream. This class would inherit from raw_fd_ostream and it would take an additional constructor argument -- the "tied" stream. The only special thing it would do is that in its write_impl implementation, it would call TiedStream.flush() before doing the actual write. llvm::errs() would be of type tied_raw_fd_ostream, tied to outs(), everything else would remain unchanged.

Implementing this idea should be very easy. The result won't be as flexible as the std::iostream solution, but that might actually be a good thing. And as long as an application writes only/mostly to outs(), the performance impact should be zero. If an application combines stdout and stderr a lot, then it will suffer some performance degradation, but hopefully that is not the case we need to optimize for.

[jhenderson]

I'm happy to go for a tied stream approach. That would seem like a more scalable solution for clients that want to keep things in sync. In practice, not that much prints to stderr, so performance is probably not so critical, in my opinion.

I hear @JDevlieghere's concerns about messages being self-descriptive enough to not need worrying about syncing, and I agree with the principle. Indeed, I often push for more context to be included in error messages. However, my take on it is that if error messages are near related output when the two are combined, it makes it much easier to associate one with the other, regardless of how much context there is in the message. I have often found cases in build systems etc which produce huge amounts of output for whatever reason, where the error messages end up miles away from the actual related output. Often, the stdout might say something like "Build Failed" but the related stderr error messages are several hundred lines earlier, and therefore quite hard to find. Admittedly, this is as much to do with there being too much output as it is to do with flushing, but the principle still more or less applies.

In terms of the testing, I have a patch that I hope to put up for review today, to reposition the testing of the error messages in debug_line_invalid.test to be alongside their corresponding tables. The information in most (all?) of the messages is sufficient to already associate them correctly, since they contain the file offsets, but that doesn't make the test script easy to read or follow from my experience, and that's despite having written most of this test myself! Whilst I could continue to keep the errors in a single file and test that indepdendently, but with the FileCheck lines interleaved appropriately, I feel like this may get a little confusing at times.

From a purely llvm-dwarfdump perspective, I think there are likely three common usages:

1. Both stream printed in a console window - in this case, stream interleaving isn't great, as it can mean the error/warnings don't appear near corresponding stdout, meaning a warning might be missed/thought to be related to the wrong thing.
2. Both streams redirected to the same file - same as above.
3. One stream redirected to a file/both redirected to different files - error messages need enough context (which I think they have already for the most part).

In the first two cases, this patch improves the issue, whilst in the third, the only thing that happens is a slight degradation in performance in the event of a warning/error being emitted. Since these should be uncommon, I think that's harmless. Testing typically will use either the first or third version (currently uses the third, but I'd like to move to the first).

[jhenderson]

I've posted D80989 which includes test changes that rely on this flushing or something equivalent. It's also related to output ordering, but I think is slightly more general than just error messages itself.

[jhenderson]

I've tried getting the tied_raw_fd_ostream approach to work, by tying errs() to outs() as suggested, but it doesn't work when it comes to testing D80989 (and possibly others) in lit. The problem is that llvm-dwarfdump uses a ToolOutputFile, which contains its own raw_fd_ostream pointing to stdout, with its own buffer independent of the version that is provided by outs(). Consequently, the flush triggered by writing to errs() flushes the empty outs() buffer, before writing the error message, rather than the real output buffer provided by the ToolOutputFile. I see three possible ways forward: 1) Go ahead with my existing approach as suggested in the current patch (my preference), 2) abandon the whole idea of keeping the two in sync (I don't like this idea for reasons already outlined), or 3) have raw_fd_ostream instances contructed with - (the stdout variant) actually use the outs() stream, rather than its own one - this will prevent potential weird situations where we have two independent streams with their own buffers both writing to stdout, and therefore potentially getting very weird interleaving, but I'm not sure of the knock-on effects of this yet.

Thoughts?

[jhenderson]

Addressed @MaskRay's comments.

[labath]

Hm... that is unfortunate. I think that option (3) is the most correct approach -- I find the current ToolOutputFile behavior fairly problematic -- but you're right that it can have some unexpected knock-on effects, and so I'm not sure if this patch should be predicated on that.

(Incidentally, I was shocked to learn that the magicness of "-" is embedded directly into the raw_os_ostream constructor -- I would have expected this to be handled somewhere higher up (like the ToolOutputFile class, for instance).)

[JDevlieghere]

In D80803#2068152, @labath wrote:

So, combining stdout and stderr is an issue that a lot of people considered important, and indeed there are a lot of tools out there which get it "right".

I hope I didn't give the impression that I don't think it's important. I think it's really useful to have stdout and stderr for exactly the reason that James mentioned. Everything I said was weighing those benefits agains us having to maintain flushes in dwarfdump.

llvm streams have no "tied stream" concept, and I would stipulate that this is the real reason for why we're running into the problems here. The simplest and least obtrusive way this concept could be retrofitted into the existing llvm streams is to have a new class -- let's call it tied_raw_fd_ostream. This class would inherit from raw_fd_ostream and it would take an additional constructor argument -- the "tied" stream. The only special thing it would do is that in its write_impl implementation, it would call TiedStream.flush() before doing the actual write. llvm::errs() would be of type tied_raw_fd_ostream, tied to outs(), everything else would remain unchanged.

Implementing this idea should be very easy. The result won't be as flexible as the std::iostream solution, but that might actually be a good thing. And as long as an application writes only/mostly to outs(), the performance impact should be zero. If an application combines stdout and stderr a lot, then it will suffer some performance degradation, but hopefully that is not the case we need to optimize for.

I really like this idea because it takes the tools out of the business of flushing and provides a structural solution to the problem. My main issue with the original patch was that it's very tricky to cover all cases and you can't sprinkle flushes until it works. Having a tied_raw_fd_ostream would solve that issue completely.

In D80803#2068317, @jhenderson wrote:

I'm happy to go for a tied stream approach. That would seem like a more scalable solution for clients that want to keep things in sync. In practice, not that much prints to stderr, so performance is probably not so critical, in my opinion.

I hear @JDevlieghere's concerns about messages being self-descriptive enough to not need worrying about syncing, and I agree with the principle. Indeed, I often push for more context to be included in error messages. However, my take on it is that if error messages are near related output when the two are combined, it makes it much easier to associate one with the other, regardless of how much context there is in the message. I have often found cases in build systems etc which produce huge amounts of output for whatever reason, where the error messages end up miles away from the actual related output. Often, the stdout might say something like "Build Failed" but the related stderr error messages are several hundred lines earlier, and therefore quite hard to find. Admittedly, this is as much to do with there being too much output as it is to do with flushing, but the principle still more or less applies.

As I said above I completely agree with all of this!

In terms of the testing, I have a patch that I hope to put up for review today, to reposition the testing of the error messages in debug_line_invalid.test to be alongside their corresponding tables. The information in most (all?) of the messages is sufficient to already associate them correctly, since they contain the file offsets, but that doesn't make the test script easy to read or follow from my experience, and that's despite having written most of this test myself! Whilst I could continue to keep the errors in a single file and test that indepdendently, but with the FileCheck lines interleaved appropriately, I feel like this may get a little confusing at times.

I strongly believe that maintainability of the tests is very important. My point was more about making sure we don't start relying on the order. But you're right, we've been doing a great job and nothing indicates that will change, so it was more of a concern than an actual problem.

From a purely llvm-dwarfdump perspective, I think there are likely three common usages:

1. Both stream printed in a console window - in this case, stream interleaving isn't great, as it can mean the error/warnings don't appear near corresponding stdout, meaning a warning might be missed/thought to be related to the wrong thing.
2. Both streams redirected to the same file - same as above.
3. One stream redirected to a file/both redirected to different files - error messages need enough context (which I think they have already for the most part).

In the first two cases, this patch improves the issue, whilst in the third, the only thing that happens is a slight degradation in performance in the event of a warning/error being emitted. Since these should be uncommon, I think that's harmless. Testing typically will use either the first or third version (currently uses the third, but I'd like to move to the first).

In D80803#2068799, @jhenderson wrote:

I've tried getting the tied_raw_fd_ostream approach to work, by tying errs() to outs() as suggested, but it doesn't work when it comes to testing D80989 (and possibly others) in lit. The problem is that llvm-dwarfdump uses a ToolOutputFile, which contains its own raw_fd_ostream pointing to stdout, with its own buffer independent of the version that is provided by outs(). Consequently, the flush triggered by writing to errs() flushes the empty outs() buffer, before writing the error message, rather than the real output buffer provided by the ToolOutputFile. I see three possible ways forward: 1) Go ahead with my existing approach as suggested in the current patch (my preference), 2) abandon the whole idea of keeping the two in sync (I don't like this idea for reasons already outlined), or 3) have raw_fd_ostream instances contructed with - (the stdout variant) actually use the outs() stream, rather than its own one - this will prevent potential weird situations where we have two independent streams with their own buffers both writing to stdout, and therefore potentially getting very weird interleaving, but I'm not sure of the knock-on effects of this yet.

Thoughts?

My vote goes to (3) as well, though like Pavel I don't think it should block this patch. I can live with (1) for now and making (3) happen when similar issues pop up down the road.

[jhenderson]

I'm going to focus on getting my other queued-up patches up for review before I start looking at option 3. If people are happy for this to go in, it will help with me landing the other patches I have planned, but probably isn't actually required and I can probably workaround things to some extent, so can wait if that's the preferred approach. Just let me know!

[labath]

In D80803#2069014, @JDevlieghere wrote:

In D80803#2068152, @labath wrote:

So, combining stdout and stderr is an issue that a lot of people considered important, and indeed there are a lot of tools out there which get it "right".

I hope I didn't give the impression that I don't think it's important. I think it's really useful to have stdout and stderr for exactly the reason that James mentioned. Everything I said was weighing those benefits agains us having to maintain flushes in dwarfdump.

Cool. Thanks for clearing that up.

In D80803#2070721, @jhenderson wrote:

I'm going to focus on getting my other queued-up patches up for review before I start looking at option 3. If people are happy for this to go in, it will help with me landing the other patches I have planned, but probably isn't actually required and I can probably workaround things to some extent, so can wait if that's the preferred approach. Just let me know!

Since I started this whole thing, I felt like I should also chip in a bit. I've created D81078 to do the ToolOutputFile refactor. The changes are fairly straight-forward, and they don't break any of the in-tree tests. I have no idea whether they will break some strange out-of-tree use case (I feel it's relatively safe though the potential for it is definitely there...), but I suppose there's only one way to find that out...

[jhenderson]

Rebase on top of D81156. This patch now just covers removing the flushes and a bit of testing.

[MaskRay]

IIUC,

3. have raw_fd_ostream instances contructed with - (the stdout variant) actually use the outs() stream, rather than its own one - this will prevent potential weird situations where we have two independent streams with their own buffers both writing to stdout, and therefore potentially getting very weird interleaving, but I'm not sure of the knock-on effects of this yet.

has been resolved by D81078

tied_raw_fd_ostream

The idea will be implemented in D81156.

This rebased patch looks good.

[jhenderson]

@dblaikie, are you happy with this patch in its current form?

[dblaikie]

Sounds alright

[jhenderson]

This comment was removed by jhenderson.

[jhenderson]

Restore diff (I accidentally uploaded it to the wrong patch).

[JDevlieghere]

Nice :-)

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

struct LineTable {
 
     void dump(raw_ostream &OS, DIDumpOptions DumpOptions) const;
     void clear();
 
     /// Parse prologue and all rows.
     Error parse(DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
                 const DWARFContext &Ctx, const DWARFUnit *U,
                 function_ref<void(Error)> RecoverableErrorHandler,
-                raw_ostream *OS = nullptr);
+                raw_ostream *OS = nullptr, bool Verbose = false);
 
     using RowVector = std::vector<Row>;
     using RowIter = RowVector::const_iterator;
     using SequenceVector = std::vector<Sequence>;
     using SequenceIter = SequenceVector::const_iterator;
 
     struct Prologue Prologue;
     RowVector Rows;
public:
     /// handlers.
     ///
     /// \param RecoverableErrorHandler - any issues that don't prevent further
     /// parsing of the table will be reported through this handler.
     /// \param UnrecoverableErrorHandler - any issues that prevent further
     /// parsing of the table will be reported through this handler.
     /// \param OS - if not null, the parser will print information about the
     /// table as it parses it.
+    /// \param Verbose - if true, the parser will print verbose information when
+    /// printing to the output.
     LineTable parseNext(function_ref<void(Error)> RecoverableErrorHandler,
                         function_ref<void(Error)> UnrecoverableErrorHandler,
-                        raw_ostream *OS = nullptr);
+                        raw_ostream *OS = nullptr, bool Verbose = false);
 
     /// Skip the current line table and go to the following line table (if
     /// present) immediately.
     ///
     /// \param RecoverableErrorHandler - report any recoverable prologue
     /// parsing issues via this handler.
     /// \param UnrecoverableErrorHandler - report any unrecoverable prologue
     /// parsing issues via this handler.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

auto DumpLineSection = [&](DWARFDebugLine::SectionParser Parser,
                              Optional<uint64_t> DumpOffset) {
     while (!Parser.done()) {
       if (DumpOffset && Parser.getOffset() != *DumpOffset) {
         Parser.skip(DumpOpts.WarningHandler, DumpOpts.WarningHandler);
         continue;
       }
       OS << "debug_line[" << format("0x%8.8" PRIx64, Parser.getOffset())
          << "]\n";
-      if (DumpOpts.Verbose) {
-        Parser.parseNext(DumpOpts.WarningHandler, DumpOpts.WarningHandler, &OS);
-      } else {
-        DWARFDebugLine::LineTable LineTable =
-            Parser.parseNext(DumpOpts.WarningHandler, DumpOpts.WarningHandler);
-        LineTable.dump(OS, DumpOpts);
-      }
+      Parser.parseNext(DumpOpts.WarningHandler, DumpOpts.WarningHandler, &OS,
+                       DumpOpts.Verbose);
     }
   };
 
   if (const auto *Off = shouldDump(Explicit, ".debug_line", DIDT_ID_DebugLine,
                                    DObj->getLineSection().Data)) {
     DWARFDataExtractor LineData(*DObj, DObj->getLineSection(), isLittleEndian(),
                                 0);
     DWARFDebugLine::SectionParser Parser(LineData, *this, compile_units(),

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

LineOffset =
         (AddrAdvanceResult.AdjustedOpcode % LineTable->Prologue.LineRange);
   Row.Line += LineOffset;
   return {AddrAdvanceResult.AddrDelta, LineOffset};
 }
 
 Error DWARFDebugLine::LineTable::parse(
     DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
     const DWARFContext &Ctx, const DWARFUnit *U,
-    function_ref<void(Error)> RecoverableErrorHandler, raw_ostream *OS) {
+    function_ref<void(Error)> RecoverableErrorHandler, raw_ostream *OS,
+    bool Verbose) {
+  assert((OS || !Verbose) && "cannot have verbose output without stream");
   const uint64_t DebugLineOffset = *OffsetPtr;
 
   clear();
 
   Error PrologueErr =
       Prologue.parse(DebugLineData, OffsetPtr, RecoverableErrorHandler, Ctx, U);
 
   if (OS) {
-    // The presence of OS signals verbose dumping.
     DIDumpOptions DumpOptions;
-    DumpOptions.Verbose = true;
+    DumpOptions.Verbose = Verbose;
     Prologue.dump(*OS, DumpOptions);
   }
 
   if (PrologueErr)
     return PrologueErr;
 
   uint64_t ProgramLength = Prologue.TotalLength + Prologue.sizeofTotalLength();
   if (!DebugLineData.isValidOffsetForDataOfSize(DebugLineOffset,
else
     assert(Prologue.getAddressSize() == 0 ||
            Prologue.getAddressSize() == TableData.getAddressSize());
 
   ParsingState State(this, DebugLineOffset, RecoverableErrorHandler);
 
   *OffsetPtr = DebugLineOffset + Prologue.getLength();
   if (OS && *OffsetPtr < EndOffset) {
     *OS << '\n';
-    Row::dumpTableHeader(*OS, 12);
+    Row::dumpTableHeader(*OS, /*Indent=*/Verbose ? 12 : 0);
   }
   while (*OffsetPtr < EndOffset) {
-    if (OS)
+    if (Verbose)
       *OS << format("0x%08.08" PRIx64 ": ", *OffsetPtr);
 
     uint64_t OpcodeOffset = *OffsetPtr;
     uint8_t Opcode = TableData.getU8(OffsetPtr);
 
-    if (OS)
+    if (Verbose)
       *OS << format("%02.02" PRIx8 " ", Opcode);
 
     if (Opcode == 0) {
       // Extended Opcodes always start with a zero opcode followed by
       // a uleb128 length so you can skip ones you don't know about
       DataExtractor::Cursor Cursor(*OffsetPtr);
       uint64_t Len = TableData.getULEB128(Cursor);
       uint64_t ExtOffset = Cursor.tell();
 
       // Tolerate zero-length; assume length is correct and soldier on.
       if (Len == 0) {
-        if (OS)
+        if (Verbose)
           *OS << "Badly formed extended line op (length 0)\n";
         if (!Cursor)
           RecoverableErrorHandler(Cursor.takeError());
         *OffsetPtr = Cursor.tell();
         continue;
       }
 
       uint8_t SubOpcode = TableData.getU8(Cursor);
-      if (OS)
+      if (Verbose)
         *OS << LNExtendedString(SubOpcode);
       switch (SubOpcode) {
       case DW_LNE_end_sequence:
         // Set the end_sequence register of the state machine to true and
         // append a row to the matrix using the current values of the
         // state-machine registers. Then reset the registers to the initial
         // values specified above. Every statement program sequence must end
         // with a DW_LNE_end_sequence instruction which creates a row whose
         // address is that of the byte after the last target machine instruction
         // of the sequence.
         State.Row.EndSequence = true;
-        if (OS) {
+        if (Verbose) {
           *OS << "\n";
           OS->indent(12);
-          State.Row.dump(*OS);
         }
+        if (OS)
+          State.Row.dump(*OS);
         State.appendRowToMatrix();
         State.resetRowAndSequence();
         break;
 
       case DW_LNE_set_address:
         // Takes a single relocatable address as an operand. The size of the
         // operand is the size appropriate to hold an address on the target
         // machine. Set the address register to the value given by the
if (Opcode == 0) {
             State.Row.Address.Address = TableData.getRelocatedAddress(
                 Cursor, &State.Row.Address.SectionIndex);
 
             // Restore the address size if the extractor already had it.
             if (ExtractorAddressSize != 0)
               TableData.setAddressSize(ExtractorAddressSize);
           }
 
-          if (OS)
+          if (Verbose)
             *OS << format(" (0x%16.16" PRIx64 ")", State.Row.Address.Address);
         }
         break;
 
       case DW_LNE_define_file:
         // Takes 4 arguments. The first is a null terminated string containing
         // a source file name. The second is an unsigned LEB128 number
         // representing the directory index of the directory in which the file
if (Opcode == 0) {
           FileNameEntry FileEntry;
           const char *Name = TableData.getCStr(Cursor);
           FileEntry.Name =
               DWARFFormValue::createFromPValue(dwarf::DW_FORM_string, Name);
           FileEntry.DirIdx = TableData.getULEB128(Cursor);
           FileEntry.ModTime = TableData.getULEB128(Cursor);
           FileEntry.Length = TableData.getULEB128(Cursor);
           Prologue.FileNames.push_back(FileEntry);
-          if (OS)
+          if (Verbose)
             *OS << " (" << Name << ", dir=" << FileEntry.DirIdx << ", mod_time="
                 << format("(0x%16.16" PRIx64 ")", FileEntry.ModTime)
                 << ", length=" << FileEntry.Length << ")";
         }
         break;
 
       case DW_LNE_set_discriminator:
         State.Row.Discriminator = TableData.getULEB128(Cursor);
-        if (OS)
+        if (Verbose)
           *OS << " (" << State.Row.Discriminator << ")";
         break;
 
       default:
-        if (OS)
+        if (Verbose)
           *OS << format("Unrecognized extended op 0x%02.02" PRIx8, SubOpcode)
               << format(" length %" PRIx64, Len);
         // Len doesn't include the zero opcode byte or the length itself, but
         // it does include the sub_opcode, so we have to adjust for that.
         TableData.skip(Cursor, Len - 1);
         break;
       }
       // Make sure the length as recorded in the table and the standard length
       // for the opcode match. If they don't, continue from the end as claimed
       // by the table. Similarly, continue from the claimed end in the event of
       // a parsing error.
       uint64_t End = ExtOffset + Len;
       if (!Cursor)
         RecoverableErrorHandler(Cursor.takeError());
       else if (Cursor.tell() != End)
         RecoverableErrorHandler(createStringError(
             errc::illegal_byte_sequence,
             "unexpected line op length at offset 0x%8.8" PRIx64
             " expected 0x%2.2" PRIx64 " found 0x%2.2" PRIx64,
             ExtOffset, Len, Cursor.tell() - ExtOffset));
       *OffsetPtr = End;
     } else if (Opcode < Prologue.OpcodeBase) {
-      if (OS)
+      if (Verbose)
         *OS << LNStandardString(Opcode);
       switch (Opcode) {
       // Standard Opcodes
       case DW_LNS_copy:
         // Takes no arguments. Append a row to the matrix using the
         // current values of the state-machine registers.
-        if (OS) {
+        if (Verbose) {
           *OS << "\n";
           OS->indent(12);
+        }
+        if (OS)
           State.Row.dump(*OS);
+        if (Verbose) // FIXME: Don't print this newline.
           *OS << "\n";
-        }
         State.appendRowToMatrix();
         break;
 
       case DW_LNS_advance_pc:
         // Takes a single unsigned LEB128 operand, multiplies it by the
         // min_inst_length field of the prologue, and adds the
         // result to the address register of the state machine.
         {
           uint64_t AddrOffset = State.advanceAddr(
               TableData.getULEB128(OffsetPtr), Opcode, OpcodeOffset);
-          if (OS)
+          if (Verbose)
             *OS << " (" << AddrOffset << ")";
         }
         break;
 
       case DW_LNS_advance_line:
         // Takes a single signed LEB128 operand and adds that value to
         // the line register of the state machine.
         State.Row.Line += TableData.getSLEB128(OffsetPtr);
-        if (OS)
+        if (Verbose)
           *OS << " (" << State.Row.Line << ")";
         break;
 
       case DW_LNS_set_file:
         // Takes a single unsigned LEB128 operand and stores it in the file
         // register of the state machine.
         State.Row.File = TableData.getULEB128(OffsetPtr);
-        if (OS)
+        if (Verbose)
           *OS << " (" << State.Row.File << ")";
         break;
 
       case DW_LNS_set_column:
         // Takes a single unsigned LEB128 operand and stores it in the
         // column register of the state machine.
         State.Row.Column = TableData.getULEB128(OffsetPtr);
-        if (OS)
+        if (Verbose)
           *OS << " (" << State.Row.Column << ")";
         break;
 
       case DW_LNS_negate_stmt:
         // Takes no arguments. Set the is_stmt register of the state
         // machine to the logical negation of its current value.
         State.Row.IsStmt = !State.Row.IsStmt;
         break;
if (Opcode == 0) {
         // twice the range of the last special opcode, it can use
         // DW_LNS_const_add_pc followed by a special opcode, for a total
         // of two bytes. Only if it needs to advance the address by more
         // than twice that range will it need to use both DW_LNS_advance_pc
         // and a special opcode, requiring three or more bytes.
         {
           uint64_t AddrOffset =
               State.advanceAddrForOpcode(Opcode, OpcodeOffset).AddrDelta;
-          if (OS)
+          if (Verbose)
             *OS << format(" (0x%16.16" PRIx64 ")", AddrOffset);
         }
         break;
 
       case DW_LNS_fixed_advance_pc:
         // Takes a single uhalf operand. Add to the address register of
         // the state machine the value of the (unencoded) operand. This
         // is the only extended opcode that takes an argument that is not
         // a variable length number. The motivation for DW_LNS_fixed_advance_pc
         // is this: existing assemblers cannot emit DW_LNS_advance_pc or
         // special opcodes because they cannot encode LEB128 numbers or
         // judge when the computation of a special opcode overflows and
         // requires the use of DW_LNS_advance_pc. Such assemblers, however,
         // can use DW_LNS_fixed_advance_pc instead, sacrificing compression.
         {
           uint16_t PCOffset = TableData.getRelocatedValue(2, OffsetPtr);
           State.Row.Address.Address += PCOffset;
-          if (OS)
-            *OS
-                << format(" (0x%4.4" PRIx16 ")", PCOffset);
+          if (Verbose)
+            *OS << format(" (0x%4.4" PRIx16 ")", PCOffset);
         }
         break;
 
       case DW_LNS_set_prologue_end:
         // Takes no arguments. Set the prologue_end register of the
         // state machine to true
         State.Row.PrologueEnd = true;
         break;
 
       case DW_LNS_set_epilogue_begin:
         // Takes no arguments. Set the basic_block register of the
         // state machine to true
         State.Row.EpilogueBegin = true;
         break;
 
       case DW_LNS_set_isa:
         // Takes a single unsigned LEB128 operand and stores it in the
         // column register of the state machine.
         State.Row.Isa = TableData.getULEB128(OffsetPtr);
-        if (OS)
+        if (Verbose)
           *OS << " (" << (uint64_t)State.Row.Isa << ")";
         break;
 
       default:
         // Handle any unknown standard opcodes here. We know the lengths
         // of such opcodes because they are specified in the prologue
         // as a multiple of LEB128 operands for each opcode.
         {
           assert(Opcode - 1U < Prologue.StandardOpcodeLengths.size());
           uint8_t OpcodeLength = Prologue.StandardOpcodeLengths[Opcode - 1];
           for (uint8_t I = 0; I < OpcodeLength; ++I) {
             uint64_t Value = TableData.getULEB128(OffsetPtr);
-            if (OS)
+            if (Verbose)
               *OS << format("Skipping ULEB128 value: 0x%16.16" PRIx64 ")\n",
                             Value);
           }
         }
         break;
       }
     } else {
       // Special Opcodes.
       ParsingState::AddrAndLineDelta Delta =
           State.handleSpecialOpcode(Opcode, OpcodeOffset);
 
-      if (OS) {
+      if (Verbose) {
         *OS << "address += " << Delta.Address << ",  line += " << Delta.Line
             << "\n";
         OS->indent(12);
-        State.Row.dump(*OS);
       }
+      if (OS)
+        State.Row.dump(*OS);
 
       State.appendRowToMatrix();
     }
-    if(OS)
+    if(Verbose)
       *OS << "\n";
   }
 
   if (!State.Sequence.Empty)
     RecoverableErrorHandler(createStringError(
         errc::illegal_byte_sequence,
         "last sequence in debug line table at offset 0x%8.8" PRIx64
         " is not terminated",
         DebugLineOffset));
 
   // Sort all sequences so that address lookup will work faster.
   if (!Sequences.empty()) {
     llvm::sort(Sequences, Sequence::orderByHighPC);
     // Note: actually, instruction address ranges of sequences should not
     // overlap (in shared objects and executables). If they do, the address
     // lookup would still work, though, but result would be ambiguous.
     // We don't report warning in this case. For example,
     // sometimes .so compiled from multiple object files contains a few
     // rudimentary sequences for address ranges [0x0, 0xsomething).
   }
 
+  // Terminate the table with a final blank line to clearly delineate it from
+  // later dumps.
+  if (OS)
+    *OS << "\n";
+
   return Error::success();
 }
 
 uint32_t DWARFDebugLine::LineTable::findRowInSeq(
     const DWARFDebugLine::Sequence &Seq,
     object::SectionedAddress Address) const {
   if (!Seq.containsPC(Address))
     return UnknownRowIndex;

 }
 
 bool DWARFDebugLine::Prologue::totalLengthIsValid() const {
   return TotalLength != 0u;
 }
 
 DWARFDebugLine::LineTable DWARFDebugLine::SectionParser::parseNext(
     function_ref<void(Error)> RecoverableErrorHandler,
-    function_ref<void(Error)> UnrecoverableErrorHandler, raw_ostream *OS) {
+    function_ref<void(Error)> UnrecoverableErrorHandler, raw_ostream *OS,
+    bool Verbose) {
   assert(DebugLineData.isValidOffset(Offset) &&
          "parsing should have terminated");
   DWARFUnit *U = prepareToParse(Offset);
   uint64_t OldOffset = Offset;
   LineTable LT;
   if (Error Err = LT.parse(DebugLineData, &Offset, Context, U,
-                           RecoverableErrorHandler, OS))
+                           RecoverableErrorHandler, OS, Verbose))
     UnrecoverableErrorHandler(std::move(Err));
   moveToNextTable(OldOffset, LT.Prologue);
   return LT;
 }
 
 void DWARFDebugLine::SectionParser::skip(
     function_ref<void(Error)> RecoverableErrorHandler,
     function_ref<void(Error)> UnrecoverableErrorHandler) {

llvm/test/tools/llvm-dwarfdump/X86/debug_line_dwarf64_large_table.s

 ## Test that we can dump the (intact) prologue of a large table which was
 ## truncated. Also, make sure we don't get confused by a DWARF64 length which
 ## matches one of the reserved initial length values.
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s >%t
 # RUN: llvm-dwarfdump %t -debug-line 2>&1 | FileCheck %s
 
 # CHECK: debug_line[0x00000000]
-# CHECK-NEXT: warning: line table program with offset 0x00000000 has length 0xfffffffc but only 0x0000003a bytes are available
 # CHECK-NEXT: Line table prologue:
 # CHECK-NEXT:     total_length: 0x00000000fffffff0
 # CHECK-NEXT:           format: DWARF64
 # CHECK-NEXT:          version: 4
 # CHECK-NEXT:  prologue_length: 0x0000000000000016
+# CHECK:        file_names[ 1]:
+# CHECK-NEXT:             name: "file1"
+# CHECK-NEXT:        dir_index: 0
+# CHECK-NEXT:         mod_time: 0x00000000
+# CHECK-NEXT:           length: 0x00000000
+# CHECK-NEXT: warning: line table program with offset 0x00000000 has length 0xfffffffc but only 0x0000003a bytes are available
 
 # CHECK:      0x000000000badbeef      1      0      1   0             0  is_stmt end_sequence
 
 .section .debug_line,"",@progbits
 .long   0xffffffff      # Length of Unit (DWARF-64 format)
 .quad   0xfffffff0
 .short  4               # DWARF version number
 .quad   .Lprologue1_end-.Lprologue1_start # Length of Prologue

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

 # RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,NOLATER,RESERVED
 
 ## Stop looking for the specified unit, if a fatally-bad prologue is detected.
 # RUN: llvm-dwarfdump -debug-line=0x4b %t-reserved.o 2>&1 \
 # RUN:   | FileCheck %s --check-prefixes=HEADER,NOFIRST,NOLATER,RESERVED
 
 ## Show that non-fatal errors do not prevent parsing the rest of the section.
 # RUN: llvm-mc -triple x86_64-pc-linux %S/Inputs/debug_line_malformed.s -filetype=obj -o %t-malformed.o
-# RUN: llvm-dwarfdump -debug-line %t-malformed.o 2> %t-malformed.err \
-# RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,NONFATAL,LAST --implicit-check-not='debug_line[{{.*}}]'
-# RUN: FileCheck %s --input-file=%t-malformed.err --check-prefixes=ALL,OTHER
-# RUN: llvm-dwarfdump -debug-line %t-malformed.o -verbose 2> %t-malformed-verbose.err \
-# RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,VERBOSE,NONFATAL,LAST --implicit-check-not='debug_line[{{.*}}]' \
-# RUN:       --implicit-check-not=' DW_LNS' --implicit-check-not=' DW_LNE' --implicit-check-not='address +='
-# RUN: FileCheck %s --input-file=%t-malformed-verbose.err --check-prefixes=ALL,OTHER
+# RUN: llvm-dwarfdump -debug-line %t-malformed.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,NONFATAL,LAST,SOME-ERR,MORE-ERR \
+# RUN:       --implicit-check-not='debug_line[{{.*}}]' --implicit-check-not='warning:'
+# RUN: llvm-dwarfdump -debug-line %t-malformed.o -verbose 2>&1 \
+# RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,VERBOSE,NONFATAL,LAST,SOME-ERR,MORE-ERR \
+# RUN:       --implicit-check-not='debug_line[{{.*}}]' --implicit-check-not=' DW_LNS' \
+# RUN:       --implicit-check-not=' DW_LNE' --implicit-check-not='address +=' \
+# RUN:       --implicit-check-not='warning:'
 
 ## We should still produce warnings for malformed tables after the specified unit.
-# RUN: llvm-dwarfdump -debug-line=0 %t-malformed.o 2> %t-malformed-off-first.err \
-# RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,NOLATER
-# RUN: FileCheck %s --input-file=%t-malformed-off-first.err --check-prefix=ALL
+# RUN: llvm-dwarfdump -debug-line=0 %t-malformed.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefixes=HEADER,FIRST,NOLATER,SOME-ERR --implicit-check-not='warning:'
 
 ## Don't stop looking for the later unit if non-fatal issues are found.
-# RUN: llvm-dwarfdump -debug-line=0x419 %t-malformed.o 2> %t-malformed-off-last.err \
-# RUN:   | FileCheck %s --check-prefixes=HEADER,LAST --implicit-check-not='debug_line[{{.*}}]'
-# RUN: FileCheck %s --input-file=%t-malformed-off-last.err --check-prefix=ALL
+# RUN: llvm-dwarfdump -debug-line=0x419 %t-malformed.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefixes=HEADER,LAST,SOME-ERR --implicit-check-not='debug_line[{{.*}}]' \
+# RUN:       --implicit-check-not='warning:'
 
 # HEADER:       .debug_line contents:
 
 # FIRST:        debug_line[0x00000000]
 # VERBOSE:        Address            Line   Column File   ISA Discriminator Flags
 # VERBOSE-NEXT:   ------------------ ------ ------ ------ --- ------------- -------------
 # VERBOSE-NEXT: DW_LNE_set_address (0x000000000badbeef)
 # VERBOSE-NEXT: DW_LNE_end_sequence
 # FIRST:        0x000000000badbeef {{.*}} end_sequence
+# FIRST-EMPTY:
 # NOFIRST-NOT:  debug_line[0x00000000]
 # NOFIRST-NOT:  0x000000000badbeef {{.*}} end_sequence
 # NOLATER-NOT:  debug_line[{{.*}}]
 # NOLATER-NOT:  end_sequence
 
 ## For fatal issues, the following table(s) should not be dumped:
 # FATAL:      debug_line[0x00000048]
 # RESERVED-NOT: prologue
 # RESERVED: warning: parsing line table prologue at offset 0x00000048: unsupported reserved unit length of value 0xfffffffe
 # RESERVED-NOT: prologue
 # RESERVED-NOT: Address
 # FATAL-NOT:  debug_line
 
 ## For non-fatal issues, the table data should be dumped:
 
 ## Version 0 table.
 # NONFATAL:      debug_line[0x00000048]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL-NEXT:    total_length: 0x00000002
 # NONFATAL-NEXT:          format: DWARF32
 # NONFATAL-NEXT:         version: 0
+# SOME-ERR-NEXT: warning: parsing line table prologue at offset 0x00000048: unsupported version 0
 # NONFATAL-NOT: prologue_length
 
 ## Version 1 table.
 # NONFATAL:      debug_line[0x0000004e]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL-NEXT:    total_length: 0x00000002
 # NONFATAL-NEXT:          format: DWARF32
 # NONFATAL-NEXT:         version: 1
+# SOME-ERR-NEXT: warning: parsing line table prologue at offset 0x0000004e: unsupported version 1
 # NONFATAL-NOT: prologue_length
 
 ## Malformed directory format with no path component.
 # NONFATAL:      debug_line[0x00000054]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
+# SOME-ERR-NEXT: warning: failed to parse entry content descriptions because no path was found
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      prologue_length: 0x00000013
 # NONFATAL-NOT:  include_directories
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0x8877665544332211)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## Prologue with length shorter than parsed.
 # NONFATAL:      debug_line[0x00000081]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x00000081 found an invalid directory or file table description at 0x000000ba
+# SOME-ERR-NEXT: warning: file names table was not null terminated before the end of the prologue
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      file_names[  1]:
 # NONFATAL-NEXT:            name: "file1"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # VERBOSE:       DW_LNS_negate_stmt
 # VERBOSE-NEXT:  DW_LNE_set_address (0x1122334455667788)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## Prologue with length longer than parsed.
 # NONFATAL:      debug_line[0x000000c8]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x000000c8 should have ended at 0x00000103 but it ended at 0x00000102
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      file_names[  2]:
 # NONFATAL-NEXT:            name: "file2"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0x1111222233334444)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## Extended opcode with incorrect length versus expected.
 # NONFATAL:      debug_line[0x00000111]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      prologue_length: 0x00000030
 # VERBOSE:       DW_LNE_set_address (0x00000000abbadaba)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
+# MORE-ERR:      warning: unexpected line op length at offset 0x00000158 expected 0x02 found 0x01
 # VERBOSE:       DW_LNE_set_discriminator (10)
-# VERBOSE-NEXT:  DW_LNS_set_prologue_end
+# MORE-ERR:      warning: unexpected line op length at offset 0x0000015c expected 0x01 found 0x02
+# VERBOSE:       DW_LNS_set_prologue_end
 # VERBOSE-NEXT:  DW_LNE_set_address (0x00000000babb1e45)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## No end of sequence.
 # NONFATAL:      debug_line[0x0000016c]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      prologue_length: 0x00000030
 # VERBOSE:       DW_LNE_set_address (0x00000000deadfade)
 # VERBOSE-NEXT:  DW_LNS_copy
+# MORE-ERR:      warning: last sequence in debug line table at offset 0x0000016c is not terminated
 
 ## Very short prologue length for V5 (ends during parameters).
 # NONFATAL:      debug_line[0x000001b2]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x000001b2 should have ended at 0x000001ce but it ended at 0x000001e1
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      standard_opcode_lengths[DW_LNS_set_isa] = 1
 # NONFATAL-NEXT: include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "xyz"
 # VERBOSE:       DW_LNE_end_sequence
 # VERBOSE:       DW_LNS_copy
 # VERBOSE:       DW_LNS_const_add_pc (0x0000000000000011)
 # VERBOSE-NEXT:  DW_LNS_copy
 # VERBOSE:       address += 2, line += 1
 # VERBOSE:       address += 7, line += 0
 # VERBOSE:       address += 6, line += 7
 # VERBOSE:       address += 7, line += -4
 # VERBOSE:       DW_LNE_end_sequence
 # VERBOSE:       DW_LNS_const_add_pc (0x0000000000000011)
 # VERBOSE-NEXT:  DW_LNS_copy
 # VERBOSE:       address += 7, line += 4
 # VERBOSE:       address += 7, line += 5
 # VERBOSE:       address += 7, line += 6
 # VERBOSE:       DW_LNE_set_address (0x0000babb1ebabb1e)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## V5 prologue ends during file table.
 # NONFATAL:      debug_line[0x000001ee]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x000001ee should have ended at 0x00000219 but it ended at 0x00000220
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "xyz"
 # NONFATAL-NEXT:       dir_index: 1
 # VERBOSE:       DW_LNS_set_epilogue_begin
 # VERBOSE-NEXT:  DW_LNS_copy
 # VERBOSE:       address += 7, line += 4
 # VERBOSE:       address += 7, line += 5
 # VERBOSE:       address += 7, line += 6
 # VERBOSE:       DW_LNE_end_sequence
 # VERBOSE:       DW_LNE_set_address (0x00000ab4acadab4a)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## V5 prologue ends during directory table.
 # NONFATAL:      debug_line[0x0000022f]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x0000022f should have ended at 0x00000251 but it ended at 0x0000025e
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "xyz"
 # VERBOSE:       address += 2, line += 1
 # VERBOSE:       address += 7, line += 0
 # VERBOSE:       address += 6, line += 7
 # VERBOSE:       address += 7, line += -4
 # VERBOSE:       DW_LNE_end_sequence
 # VERBOSE:       DW_LNS_const_add_pc (0x0000000000000011)
 # VERBOSE-NEXT:  DW_LNS_copy
 # VERBOSE:       address += 7, line += 4
 # VERBOSE:       address += 7, line += 5
 # VERBOSE:       address += 7, line += 6
 # VERBOSE:       DW_LNE_set_address (0x4444333322221111)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## V5 invalid MD5 hash form when there is still data to be read.
 # NONFATAL:      debug_line[0x0000026b]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x0000026b found an invalid directory or file table description at 0x0000029f
+# SOME-ERR-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0x1234123412341234)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## V5 invalid MD5 hash form when data beyond the prologue length has
 ## been read before the MD5 problem is identified.
 # NONFATAL:      debug_line[0x000002ae]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x000002ae found an invalid directory or file table description at 0x000002e0
+# SOME-ERR-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNS_set_epilogue_begin
 # VERBOSE-NEXT:  DW_LNS_copy
 # VERBOSE:       address += 7, line += 4
 # VERBOSE:       address += 7, line += 5
 # VERBOSE:       address += 7, line += 6
 # VERBOSE:       DW_LNE_set_address (0x4321432143214321)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## V5 invalid directory content description has unsupported form.
 # NONFATAL:      debug_line[0x000002ec]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x000002ec found an invalid directory or file table description at 0x00000315
+# SOME-ERR-NEXT: warning: failed to parse directory entry because skipping the form value failed.
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/foo"
 # NONFATAL-NOT:  include_directories
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0xaaaabbbbccccdddd)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## Opcode base field of value zero.
 # NONFATAL:      debug_line[0x00000332]
+# SOME-ERR-NEXT: warning: parsing line table prologue at offset 0x00000332 found opcode base of 0. Assuming no standard opcodes
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  1] = "dir1"
 # NONFATAL-NEXT: file_names[  1]:
 # NONFATAL-NEXT:            name: "file1"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # VERBOSE:       DW_LNE_set_address (0xffffeeeeddddcccc)
 # VERBOSE-NEXT:  address += 1, line += 0
 # VERBOSE:       DW_LNE_end_sequence
 
 ## V4 table with unterminated include directory table.
 # NONFATAL:      debug_line[0x00000361]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x00000361 found an invalid directory or file table description at 0x00000383
+# SOME-ERR-NEXT: warning: include directories table was not null terminated before the end of the prologue
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  1] = "dir1"
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0xabcdef0123456789)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## V4 table with unterminated file name table.
 # NONFATAL:      debug_line[0x00000390]
+# SOME-ERR-NEXT: warning: parsing line table prologue at 0x00000390 found an invalid directory or file table description at 0x000003bf
+# SOME-ERR-NEXT: warning: file names table was not null terminated before the end of the prologue
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      file_names[  1]:
 # NONFATAL-NEXT:            name: "foo.c"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0xababcdcdefef0909)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## Table with extended opcode that overruns table end.
 # NONFATAL:      debug_line[0x000003c9]
 # NONFATAL-NEXT: Line table prologue
 # VERBOSE:       DW_LNE_set_address (0x00000000feedfeed)
 # VERBOSE-NEXT:  DW_LNS_copy
 # VERBOSE:       DW_LNE_set_address (0x0000000000000000)
+# MORE-ERR:      warning: unexpected end of data at offset 0x419 while reading [0x412, 0x41a)
+# MORE-ERR:      warning: last sequence in debug line table at offset 0x000003c9 is not terminated
 
 # LAST:          debug_line[0x00000419]
 # VERBOSE:       DW_LNE_set_address (0x00000000cafebabe)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
-
-# ALL-NOT:  warning:
-# ALL:      warning: parsing line table prologue at offset 0x00000048: unsupported version 0
-# ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e: unsupported version 1
-# ALL-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
-# ALL-NEXT: warning: failed to parse entry content descriptions because no path was found
-# ALL-NEXT: warning: parsing line table prologue at 0x00000081 found an invalid directory or file table description at 0x000000ba
-# ALL-NEXT: warning: file names table was not null terminated before the end of the prologue
-# ALL-NEXT: warning: parsing line table prologue at 0x000000c8 should have ended at 0x00000103 but it ended at 0x00000102
-# OTHER-NEXT: warning: unexpected line op length at offset 0x00000158 expected 0x02 found 0x01
-# OTHER-NEXT: warning: unexpected line op length at offset 0x0000015c expected 0x01 found 0x02
-# OTHER-NEXT: warning: last sequence in debug line table at offset 0x0000016c is not terminated
-# ALL-NEXT: warning: parsing line table prologue at 0x000001b2 should have ended at 0x000001ce but it ended at 0x000001e1
-# ALL-NEXT: warning: parsing line table prologue at 0x000001ee should have ended at 0x00000219 but it ended at 0x00000220
-# ALL-NEXT: warning: parsing line table prologue at 0x0000022f should have ended at 0x00000251 but it ended at 0x0000025e
-# ALL-NEXT: warning: parsing line table prologue at 0x0000026b found an invalid directory or file table description at 0x0000029f
-# ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
-# ALL-NEXT: warning: parsing line table prologue at 0x000002ae found an invalid directory or file table description at 0x000002e0
-# ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
-# ALL-NEXT: warning: parsing line table prologue at 0x000002ec found an invalid directory or file table description at 0x00000315
-# ALL-NEXT: warning: failed to parse directory entry because skipping the form value failed.
-# ALL-NEXT: warning: parsing line table prologue at offset 0x00000332 found opcode base of 0. Assuming no standard opcodes
-# ALL-NEXT: warning: parsing line table prologue at 0x00000361 found an invalid directory or file table description at 0x00000383
-# ALL-NEXT: warning: include directories table was not null terminated before the end of the prologue
-# ALL-NEXT: warning: parsing line table prologue at 0x00000390 found an invalid directory or file table description at 0x000003bf
-# ALL-NEXT: warning: file names table was not null terminated before the end of the prologue
-# OTHER-NEXT: warning:  unexpected end of data at offset 0x419 while reading [0x412, 0x41a)
-# OTHER-NEXT: warning: last sequence in debug line table at offset 0x000003c9 is not terminated
-# ALL-NOT:  warning:

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

TEST_F(DebugLineBasicFixture, ParserPrintsStandardOpcodesWhenRequested) {
   LT.addStandardOpcode(DW_LNS_set_epilogue_begin, {});
   LT.addStandardOpcode(DW_LNS_set_isa, {ValLen{66, LineTable::ULEB}});
   LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
   std::string Output;
   raw_string_ostream OS(Output);
-  Parser.parseNext(RecordRecoverable, RecordUnrecoverable, &OS);
+  Parser.parseNext(RecordRecoverable, RecordUnrecoverable, &OS,
+                   /*Verbose=*/true);
   OS.flush();
 
   EXPECT_FALSE(Recoverable);
   EXPECT_FALSE(Unrecoverable);
   auto InOutput = [&Output](char const *Str) {
     return Output.find(Str) != std::string::npos;
   };
   EXPECT_TRUE(InOutput("0x0000002e: 01 DW_LNS_copy\n")) << Output;
void runTest() {
     Prologue.TotalLength += BodyLength;
     LT.setPrologue(Prologue);
     LT.addExtendedOpcode(OpcodeLength, Opcode, Operands);
     generate();
 
     DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
     std::string Output;
     raw_string_ostream OS(Output);
-    Parser.parseNext(RecordRecoverable, RecordUnrecoverable, &OS);
+    Parser.parseNext(RecordRecoverable, RecordUnrecoverable, &OS,
+                     /*Verbose=*/true);
     OS.flush();
 
     StringRef LinePrefix = "0x0000002e: 00 ";
     StringRef OutputRef(Output);
     StringRef OutputToCheck = OutputRef.split(LinePrefix).second;
-    EXPECT_EQ((ExpectedOutput + "\n").str(), OutputToCheck);
+    // Each extended opcode ends with a new line and then the table ends with an
+    // additional blank line.
+    EXPECT_EQ((ExpectedOutput + "\n\n").str(), OutputToCheck);
     EXPECT_THAT_ERROR(std::move(Recoverable),
                       FailedWithMessage(ExpectedErr.str()));
   }
 
   uint64_t BodyLength;
   uint64_t OpcodeLength;
   uint8_t Opcode;
   ValueAndLengths Operands;