llvm-nm should flush and error check output

[davide]

Summary

Recent additions to the UNIX03 conformance suite have started checking that utilities properly handle output errors by closing stdout on the program and checking for an error message + exit. llvm-nm needs to explicitly flush stdout prior to a successful exit and error-check the stream.

rdar://89760992

[pete]

Wonder if we'll find other tools with the same issue. But for now its good to fix the one we know about.

[davide]

I don't run POSIX compliance, somebody just gave me a patch, but yes, good point.

[jhenderson]

The pre-merge check is failing to build this patch.

There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:

1. returm 0 is implicit at the end of main (see the accepted answer to this question: https://stackoverflow.com/questions/204476/what-should-main-return-in-c-and-c).
2. stdout is flushed on exit automatically: https://stackoverflow.com/questions/15911517/is-there-a-guarantee-of-stdout-auto-flush-before-exit-how-does-it-work

How does explicit flushing help us with output errors. More importantly, why do we care?

[davide]

In D121159#3366196, @jhenderson wrote:

The pre-merge check is failing to build this patch.

There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:

I think this is for UNIX03 compliance. Not all the tools have to be compliant.

1. returm 0 is implicit at the end of main (see the accepted answer to this question: https://stackoverflow.com/questions/204476/what-should-main-return-in-c-and-c).

Sure, I can remove it.

2. stdout is flushed on exit automatically: https://stackoverflow.com/questions/15911517/is-there-a-guarantee-of-stdout-auto-flush-before-exit-how-does-it-work

If stdout is flushed before exit, that's fine. What about stderr ?

How does explicit flushing help us with output errors. More importantly, why do we care?

We care for POSIX compliance. Apple can keep this patch downstream, but I don't see the harm of having this one here.

[jhenderson]

In D121159#3366227, @davide wrote:

In D121159#3366196, @jhenderson wrote:

The pre-merge check is failing to build this patch.

There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:

I think this is for UNIX03 compliance. Not all the tools have to be compliant.

Perhaps not, but there's presumably a good reason for compliance requirements. Anyway, I'd be surprised if llvm-nm is any more special than e.g. llvm-readelf or llvm-objdump in this area.

2. stdout is flushed on exit automatically: https://stackoverflow.com/questions/15911517/is-there-a-guarantee-of-stdout-auto-flush-before-exit-how-does-it-work

If stdout is flushed before exit, that's fine. What about stderr ?

stderr is unbuffered. It shouldn't need flushing.

How does explicit flushing help us with output errors. More importantly, why do we care?

We care for POSIX compliance. Apple can keep this patch downstream, but I don't see the harm of having this one here.

Harm comes from having code that is unnecessary, since it does nothing that doesn't automatically happen. Future developers will come along and see it and delete it because it appears to be pointless. You didn't answer the first half of that comment though: how does explicit flushing help us? The patch description talks about error checking a stream, but this modification doesn't seem to do anything of the sort.

[davide]

In D121159#3366232, @jhenderson wrote:

In D121159#3366227, @davide wrote:

In D121159#3366196, @jhenderson wrote:

The pre-merge check is failing to build this patch.

There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:

I think this is for UNIX03 compliance. Not all the tools have to be compliant.

Perhaps not, but there's presumably a good reason for compliance requirements. Anyway, I'd be surprised if llvm-nm is any more special than e.g. llvm-readelf or llvm-objdump in this area.

2. stdout is flushed on exit automatically: https://stackoverflow.com/questions/15911517/is-there-a-guarantee-of-stdout-auto-flush-before-exit-how-does-it-work

If stdout is flushed before exit, that's fine. What about stderr ?

stderr is unbuffered. It shouldn't need flushing.

How does explicit flushing help us with output errors. More importantly, why do we care?

We care for POSIX compliance. Apple can keep this patch downstream, but I don't see the harm of having this one here.

Harm comes from having code that is unnecessary, since it does nothing that doesn't automatically happen. Future developers will come along and see it and delete it because it appears to be pointless. You didn't answer the first half of that comment though: how does explicit flushing help us? The patch description talks about error checking a stream, but this modification doesn't seem to do anything of the sort.

Fair. I'll come up with a testcase where this matters (taken from the suite) and propose a new patch. For now, I'm withdrawing.

llvm/tools/llvm-nm/llvm-nm.cpp

     printExportSymbolList();
   }
 
+  // Needed for UNIX03 conformance.
+  outs.flush();
+
   if (HadError)
     return 1;
+  return 0;
 }