Make mbed_getc and gets compatible with POSIX ( and also IAR 8.x )

[Alex-EEE]

Description

I came across this while trying to get the Stream object to work in IAR 8.x. As you can see, there was already a kludge fix for IAR 7.x that touched the _FILE descriptor. This fix won't compile in 8.x so it is simply disabled. This causes streams to fail in IAR 8.x ( specifically, putc stops working after you call getc )

I tracked this down to the root cause. Basically, you are allowed in POSIX / ANSI C to read and write on the same stream, BUT, you have to do an fseek in between. From the Linux man pages ( https://www.systutorials.com/docs/linux/man/3-fdopen/ ):

"Reads and writes may be intermixed on read/write streams in any order. Note that ANSI C requires that a file positioning function intervene between output and input, unless an input operation encounters end-of-file. (If this condition is not met, then a read is allowed to return the result of writes other than the most recent.) Therefore it is good practice (and indeed sometimes necessary under Linux) to put an fseek(3) or fgetpos(3) operation between write and read operations on such a stream. This operation may be an apparent no-op (as in fseek(..., 0L, SEEK_CUR) called for its synchronizing side effect."

I did the prescribed "no op" and now it works in IAR 8.x. I would think this should also work for IAR 7.x as well ( but I don't have 7.x installed to test with) And also...whatever future POSIX platforms mbed ends up on. :-)

[ X ] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

[kjbracey]

I'm generally extremely unhappy with Stream as a concept, which is why I'd really like to kill it. #5655. It really should not be trying to treat an I/O device as a single C file - it works far better as 2, like stdin/stdout.

The fflush that Stream already does itself was supposed to be sufficient to reverse direction, but I see that it's not symmetrical. putc->fflush->getc is legal, but you actually need to do getc->fseek->putc.

But then fseek is actually usually an error on an I/O device...

Assuming that the fseek does its job of sorting out the C library buffers, and all 3 C libraries don't care if the underlying device returns an error, then this may be okay, but I think the seek should go into Stream instead of its fflush, and only on the putc.

[kjbracey]

Might need to figure this out a bit more - the standard text I'm looking at (C99) seems to contradict what you've found. Precise wording:

When a file is opened with update mode ('+' as the second or third character in the
above list of mode argument values), both input and output may be performed on the
associated stream. However, output shall not be directly followed by input without an
intervening call to the fflush function or to a file positioning function (fseek,
fsetpos, or rewind), and input shall not be directly followed by output without an
intervening call to a file positioning function, unless the input operation encounters endof-
file.

By that wording, I don't see how adding fseeks before input helps, given that Stream already does an fflush before its calls to mbed_getc and mbed_gets. Possible library bug?

I don't totally recall what that IAR workaround was about, but I'm assuming it /was/ a library bug otherwise it would have just done something through proper APIs...

[Alex-EEE]

@kjbracey-arm I see your text as agreeing with mine, not contradicting. “Input shall not be followed by output without intervening call to file positioning function “. That’s what my link says too. This commit adds the required file positioning function. IAR is in spec. The other library must be more tolerant. Am I missing something here?

[kjbracey]

Links: issue #7571 and reverting #770

[kjbracey]

Your man page is paraphrasing slightly, suggesting fseek is needed between every output and input operation in either order. C99 and POSIX standard is slightly laxer than that, asymmetrically - output,fflush,input is okay, and it's only input,fseek,output that is required and we're missing.

I then confused myself about which direction reversal you were inserting the fseek on - I wrongly thought it was adding it in the unnecessary direction. I agree this is putting in a required fseek that currently isn't happening.

However, as the actual problem is that Stream is reversing and should be fseeking, then that's the place for the fix. The simplest change should be to change the existing fflushes on entry to Stream's write functions to fseeks. The original logic of #770 was a workaround for a presumed getc bug, not to actually change the behaviour of getc to add more synchronisation than POSIX needed.

Full removal of the IAR 7 fudge itself is another issue. We'd need to test and confirm that changing fflush->fseek in Stream is a sufficient fix for what was described in #770 on IAR 7 before actually taking it out - you say you've not tested it at all. And if you were to take it the fudge, then the mbed_xxx functions should actually be removed altogether, and Stream should go back to just calling the C library - properly reverting that part of #770.

[cmonr]

@Alex-EEE @kjbracey-arm So what's the action here? It sounds like this either needs more work, more planning, or both.

[Alex-EEE]

I've got the fix in my fork and it's working fine. I think we're in agreement on the need for the fseek? I think? Seems there's discussion around the proper location for said fix.

[kjbracey]

Yes, needs work. Stream is at fault for only doing a fflush, so should be corrected, rather than the lower level working around its flaw.

And we can't remove the workaround for previous IAR as this does without any testing.

[cmonr]

@Alex-EEE Is there any more planned movement on this PR?

[0xc0170]

I believe #8331 replaces this one?

[cmonr]

I believe #8331 replaces this one

Ah! So it seems. @Alex-EEE on your confirmation, we'll close this one.

[Alex-EEE]

Yep, #8331 replaces, thanks!