Page MenuHomePhabricator

[flang] Fix output buffering bug
ClosedPublic

Authored by klausler on Mar 23 2021, 9:53 AM.

Details

Summary

The I/O runtime library code was failing to retain data in a buffer
from the current output record when flushing the buffer; this is
fatally wrong when the corresponding file cannot be repositioned,
as in the case of standard output to the console. So refine the
Flush() member function to retain a specified number of bytes,
rearrange the data as necessary (using existing code for read frame
management after moving it into a new member function), and add
a big comment to the head of the file to clarify the roles of the
various data members in the management of contiguous frames in
circular buffers.

Diff Detail

Event Timeline

klausler created this revision.Mar 23 2021, 9:53 AM
klausler requested review of this revision.Mar 23 2021, 9:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 9:53 AM

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

Yes, I am asking without fully understanding the overall infrastructure. A way to test without having to write 64KB+ of data would be nice - but not sure how easy that is to achieve either.
From what I can tell from a quick look, the buffer functionality itself isn't being tested, it is the IO functionality that is layered on top of it that gets tested - so it wouldn't be very easy to implement a dummy STORE class that tracks the actual output and expects to see certain strings - even if we could make the buffer size smaller.
Maybe the right thing would be to actually add a small test-suite for the buffer itself, along with some kind of variable setting for the buffer-size?

These are just my thoughts, as a newbie to this project.

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

Yes, I am asking without fully understanding the overall infrastructure. A way to test without having to write 64KB+ of data would be nice - but not sure how easy that is to achieve either.
From what I can tell from a quick look, the buffer functionality itself isn't being tested, it is the IO functionality that is layered on top of it that gets tested - so it wouldn't be very easy to implement a dummy STORE class that tracks the actual output and expects to see certain strings - even if we could make the buffer size smaller.
Maybe the right thing would be to actually add a small test-suite for the buffer itself, along with some kind of variable setting for the buffer-size?

These are just my thoughts, as a newbie to this project.

That's a fine idea, and I encourage you to take a whack at it. I am swamped.

PeteSteinfeld accepted this revision.Mar 23 2021, 2:04 PM

All builds, tests, and looks good.

This revision is now accepted and ready to land.Mar 23 2021, 2:04 PM

LGTM. Thanks Peter for fixing this quickly.

flang/runtime/buffer.h
31

This detailed explanation is helpful.

177

Nice function and inline documentation.

klausler updated this revision to Diff 332833.Mar 23 2021, 5:21 PM

Added a unit test.

Nice test, thanks for adding that!

LGTM

I have verified that the new test fails without the patch to buffer.h. I will run the FCVS I/O tests with the new patch before merging.