Stream: add necessary flushes, removing unneeded IAR workaround

[deepikabhavnani]

Description

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/):

Thanks @Alex-EEE for sharing the fix: #7749
Resolves: #7571

Pull request type

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

[deepikabhavnani]

@Alex-EEE - Please review

[Alex-EEE]

Looks good!

[kjbracey]

You don't need to do BOTH the flush and the seek. Correct fix is what I said on the other PR:

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.

[kjbracey]

Also, the pull title doesn't remotely match the patch, and have we actually established what the point of the mbed_getc IAR 7 hack was anyway? I don't think we got to the bottom of what #770 was about in the previous PR. You need to properly justify the removal of that workaround, and that you've retested what the original issue was on IAR 7. (If we can figure it out). I'm not totally sure #770 was purely to workaround a problem caused by Stream's lack of fseek. It was something to do with unbuffered streams in general, I thought.

Putting the fseeks in instead of flushes would be fine as-is.

[Alex-EEE]

I disagree, I think this does get to the bottom of #770. getc and putc weren't working before in IAR 7 b/c you were switching stream directions w/o making the POSIX mandated fseek. [ I guess this worked in gcc, etc b/c those compilers were more forgiving ]. The original fix was just a hack fix...it didn't address the underlying lack of fseek, but mucking with the IAR internal structure somehow make it work. This fix breaks in IAR 8 b/c that _FILE structure is ( appropriately ) hidden now, so the old fix can't see the members that it wants to mess with.

As for removing the fflush...I don't know the answer to this one way or another, but if you switch directions before all the characters have been written out, do you lose those characters? Might you still need the flush before changing directions?

[deepikabhavnani]

Also, the pull title doesn't remotely match the patch, and have we actually established what the point of the mbed_getc IAR 7 hack was anyway?

Fix #770 was related to this issue #749.
It was mix of buffering issue (dlib uses 512 bytes for read as page size) and fseek behavior. Please note mbed_set_unbuffered_stream as part of #770 fix should not be removed. This PR reverts #770 partially
https://github.com/ARMmbed/mbed-os/blob/master/platform/Stream.cpp#L31

Test case added here should be good enough to verify the same fix.

[kjbracey]

Ta @deepikabhavnani, that makes sense - this is killing the half of #770 that was to do with the lack of seek, but leaving the unbuffered stream bug workaround. I was thinking this reverted #770 entirely. Is that buffering bug still present in IAR 8?

if you switch directions before all the characters have been written out, do you lose those characters?

No (assuming IAR isn't buggy). A seek is required to flush output if necessary - streams are not permitted to arbitrarily lose data.

Anyway, the seek is going before the writes, not after. In the event of multiple back-to-back writes, ideally the seek would be a no-op (it doesn't have to flush).

[kjbracey]

Title can still be clearer - this isn't to do with "loopback", this is fixing the way Stream does its bidirectional FILE handling. Maybe Stream: add necessary flushes, removing unneeded IAR workaround

Also, this isn't really a fix, it's refactoring a fix - doing it properly rather than a tool-specific workaround. So good to have, but not the sort of thing that should be backported to 5.10.y. I can also imagine removal of mbed_getc tripping up someone. I'd mark it as refactor.

[deepikabhavnani]

Is that buffering bug still present in IAR 8?

Yes

[cmonr]

/morph build

[mbed-ci]

Build : SUCCESS

Build number : 3334
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8331/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

Build number : 2966
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8331/

[mbed-ci]

Test : SUCCESS

Build number : 3142
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8331/3142

[cmonr]

🎉