Page MenuHomePhabricator

llvm-nm should flush and error check output
AbandonedPublic

Authored by davide on Mar 7 2022, 1:50 PM.

Details

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

Diff Detail

Event Timeline

davide created this revision.Mar 7 2022, 1:50 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: rupprecht. · View Herald Transcript
davide requested review of this revision.Mar 7 2022, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 1:50 PM
pete accepted this revision.Mar 7 2022, 1:51 PM
This revision is now accepted and ready to land.Mar 7 2022, 1:51 PM
pete added a comment.Mar 7 2022, 1:51 PM

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

davide added a comment.EditedMar 7 2022, 1:52 PM

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

jhenderson requested changes to this revision.Mar 7 2022, 9:39 PM

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?

This revision now requires changes to proceed.Mar 7 2022, 9:39 PM

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.

  1. 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.

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.

  1. 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.

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.

  1. 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.

davide abandoned this revision.Mar 7 2022, 10:56 PM