-
Notifications
You must be signed in to change notification settings - Fork 3k
Add FileHandle tests #5895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add FileHandle tests #5895
Conversation
@kjbracey-arm I wrote some retargeting tests as you proposed in #5714, feel free to review. I noticed some inconsistencies between |
I think there may be a retargeting bug in partial write that you're revealing. I corrected something there in #5571, at least for ARMCC. write(0) shouldn't return -EAGAIN - 0 is more conventional. Also, you really shouldn't be returning EAGAIN anyway, as you've not been asked to be non-blocking. Basically the C library retarget doesn't really support non-blocking files - it's not clear how those calls should work in response to EAGAIN. They might just immediately retry like they would for EINTR. And as for partial writes, fwrite isn't allowed to report a partial writes except in the case of a real error. And EAGAIN (or EINTR) aren't "real" errors - there could be special code to hide them in the retargetting. I would say you should probably return something more definite indicating you're off the end of the file - EFBIG or something? It's a partial write because you actually can't write more and have hit a real error, not just some transient condition. Having done that, after that's been returned, you should be able to check ferror(FILE) is set and that errno is set to EFBIG. Don't forget to rewind() to clear the error condition. (fseek(0,SET) doesn't). |
if(SEEK_POS_IS_VALID(offset)) { | ||
_pos = offset; | ||
} else { | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return a specific error code. -EINVAL, I believe.
((uint8_t*)buffer)[read] = _data[_pos++]; | ||
} else { | ||
if(0 == read) { | ||
read = -EAGAIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're trying to act like a normal file, you should return 0 if _pos >= end of file (on entry). As in my previous comment, -EAGAIN is never appropriate for a blocking file.
If you want to test error returns, I'd have some sort of other "set_error" back door control to make it do that instead of the normal operation.
Maybe you could mark a particular byte somewhere in the file invalid, so if (_pos == _error_pos) indicate EIO
sort of thing, distinct from the "end of file". Dunno.
_fnCalled = fnRead; | ||
|
||
if(0 == size) { | ||
return -EAGAIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 is normal, so you don't need to special-case.
case SEEK_CUR: | ||
{ | ||
const int32_t new_pos = _pos + offset; | ||
if(SEEK_POS_IS_VALID(new_pos)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're repeating yourself here - switch statement could just work out new_pos, leaving the "is valid" stuff common afterwards.
5c12c5b
to
17bc9b6
Compare
test was updated (according suggestions) |
if(NEW_POS_IS_VALID(_pos)) { | ||
_data[_pos++] = ((uint8_t*)buffer)[written]; | ||
} else { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works for write - it's not symmetrical with read. You can't just stop writing data and start returning 0 - there's no "end of file" when writing. Not sure what the C libraries will do if you do this.
While in theory you can do what you like with a FileHandle (or POSIX file descriptor), the C library will be assuming somewhat conventional file-like behaviour, and that doesn't include just not making progress. The C library may well keep continuously recalling you until you return an error code, or the total amount had been written.
You will need a specific error code like -ENOSPC
here to indicate that they're writing beyond the capacity, and to make sure the C library gives up.
So I think it's return 0 if size == 0, return the amount actually written if you had to stop due to lack of space, and return -ENOSPC if can't write anything due to lack of space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
using utest::v1::Case; | ||
|
||
//#define PARTIAL_WRITE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what would it take to get this on? Do we need any tweaks from #5571? IIRC, _write()
for ARMCC is currently broken because it returns size - written
, even when written
is actually an error code.
Maybe we might want to split this into testing EOF for read, and actual errors on read/write. Those are key things that are easy to screw up, and critically important for a minority of users. So I think need to be in this test.
(Seen that in networking - very often TCP glue code can't do end-of-stream properly, which is fine for many users, but utterly breaks certain protocols).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, maybe I should start testing with changes from #5571?
Is there any sens of testing old (buggy) version while there is new one around the corner?
When it's expected to be ready for merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I recall there was only that 1 intended adjustment to _write, but maybe you could just try merging with #5571 and cross-checking results.
At the present it's not clear whether there's a test error, retarget error or C library error.
I think you do need to lock the test down to be specifically testing EOF (read) and error (read or write), as those are the only "partial" returns I would expect the C library to handle. I haven't quite fully gone through to understand what the partial code is testing at present.
Other partial returns like non-blocking or special devices are needed, but they would require someone to be accessing the FileHandle directly (or using POSIX read
/write
from #5571) - I wouldn't expect to be able to do that through the C library stdio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the ifdefs are temporarily.
Regarding write/read problem we have two scenarios:
- write more than file can handle - error occurs, we expects:
a.fwrite
should return number of objects written successfully
b.fwrite
set error state (checked then withferror
) when not all objects were written - read more than file contain -
EOF
occurs
a.fread
should return number of objects read successfully
b.fread
setEOF
when no more objects to read
I did some test on top of #5571 and get following results:
- Complete mbed_retarget FileHandle rework #5571 only fixes a bit IAR write behaviour (eliminates infinite write loop when write more than file can handle)
- Described scenarios are fulfilled only on GCC_ARM compiler
- ARM compiler results
o scenario (1) ❌fwrite
sets error, and return 0 instead of number of objects written successfully
o scenario (2) ✅fread
setsEOF
and return number of objects read successfully - IAR compiler results
o scenario (1) ❌fwrite
sets error, and return 3 instead of number of objects written successfully (5 to write, written 2, returned 3)
o scenario (2) ✅fread
sets eof and return number of objects read successfully
To sum up, looks like both ARM and IAR fwrite
implementation is broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, you're a star! Will study and ponder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that does appear to be straightforward C library bugs. Don't think we could successfully work around those.
For ARM C, we are clearly returning the number of characters not written, as the documentation requires: "A positive number representing the number of characters not written (so any nonzero return value denotes a failure of some sort)."
The ARM C library detects this is an error as expected, but somehow doesn't pass back the correct return value. And because it doesn't make a final re-call like GCC, we won't get errno set either. But setting errno isn't strictly required by fwrite/fputc. Just the return value, and setting the ferror
flag. Return value is at least a clear failure indication to app - just a bit pessimistic about where it failed.
As for IAR, um what's it doing? Can you check with some different numbers? I'd guess it's doing "successful+1" or maybe it's doing "requested-successful"? Either way, apps are in danger of false success reports (if error on last byte or first byte, respectively).
Checked the IAR docs/reference implementation, and we're doing the right thing. Doubt we could do anything to influence its behaviour, if it's calling us 1 byte at a time. Not sure why it's doing that. It's even going 1 step further and making that "0" call after every byte. According to comments in their write.c, (NULL,0) is a flush request - we should be presumably be calling sync on that. Although if it's going to do it on every byte, maybe not?
I guess they're attempting to couple fflush/nonbuffered and fsync(), but there's supposed to be a difference- fflush() flushes C library to system, and fsync() flushes system to disk. The first isn't supposed to imply the second.
Is the weird 1-at-a-time IAR behaviour maybe something to do with what mbed_set_unbuffered_stream does? I was assuming that was a bug workaround. What if you call setbuf(file, NULL) instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IAR with std::setbuf(file, NULL)
instead mbed_set_unbuffered_stream(file)
behaves more reasonably
log2.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. I actually have no idea what that IAR-specific code is supposed to achieve. It was introduced here: 9909e7c
The setvbuf
call there should be equivalent to setbuf(NULL)
, but apparently isn't. Not sure what the rest of the commit affecting the behaviour of Stream
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out some explanation is on GitHub, not the commit message: #770
Is the bug now fixed, so that can be removed? What version of IAR are you testing with?
17bc9b6
to
87897dc
Compare
@maciejbocianski I would like to understand the dependency on 5571 and master. Does it feel like 5571 should get these tests in 5571 (being part of it), that would test the changes there. Or would this be good to have tested on master (reading through the comments here there are some issues with C libs?) |
This is testing existing functionality, so it's useful to make sure #5571 isn't breaking anything. We've identified a couple of things that #5571 fixes, but there are things that are apparently faulty both before and after #5571. Could merge this PR first with some tests excluded until #5571 lands, then we could add a commit to #5571 to enable the tests it fixes. Then continue to investigate the pre-existing and continuing oddities. |
@kjbracey-arm thanks for the proposal, sounds good to me (first master) @maciejbocianski if you agree, lets make this ready for master and identify what does not work (and 5571 will fix) |
I propose we disable the fwrite tests for now, or at least relax the condition to check that error is detected - ferror set, return value less than expected. Get this merged to test #5571, then continue investigation. |
87897dc
to
55a78c8
Compare
/morph build |
Build : SUCCESSBuild number : 1036 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 716 |
Test : SUCCESSBuild number : 843 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're leaving IAR disabled, at each point I would find it useful to note in a comment what behaviour actually does exhibit at the moment.
file = fdopen(&fh, "w+"); | ||
TEST_ASSERT_NOT_NULL(file); | ||
#ifdef __ICCARM__ | ||
std::setbuf(file, NULL); // fix IAR every byte sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this then wouldn't need an ifdef. mbed_set_unbuffered_stream just calls std::setbuf(file, NULL)
if not ICCARM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in #5571 ? On master there is still std::setvbuf(_file,buf,_IONBF,NULL);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not confused, this expands to
#ifdef __ICC_ARM__
setbuf(file, NULL);
#else
// mbed_set_unbuffered_stream(file):
#if defined (__ICCARM__)
char buf[2];
setvbuf(file,buf,_IONBF,NULL);
#else
setbuf(file, NULL);
#endif
#endif
Which, more consisely is
setbuf(file, NULL);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now get it:) My idea was to preserve mbed_set_unbuffered_stream(file);
line to show that this function should be fixed by #5571 or in the future
So truncate it to single line setbuf(file, NULL);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so - the set_unbuffered_stream
(and mbed_gets
and mbed_getc
) are just some sort of IAR workaround for the normal functions setbuf
gets
and getc
at some point not working properly - a few bits of code like Stream
knew to call them instead. If we are currently thinking setbuf
is working, just use it and drop the workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
ret = std::fputc(char_buf[0], file); | ||
TEST_ASSERT_TRUE(TestFile<FS>::functionCalled(TestFile<FS>::fnWrite)); | ||
TEST_ASSERT_TRUE(std::ferror(file) > 0); | ||
std::clearerr(file); // for ARMCC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You keep calling fseek(file, 0, SEEK_SET)
. If you called rewind()
instead, that would clear the error. Should be necessary on every compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
TestFile<FS>::resetFunctionCallHistory(); | ||
ret = std::fputc(char_buf[0], file); | ||
TEST_ASSERT_TRUE(TestFile<FS>::functionCalled(TestFile<FS>::fnWrite)); | ||
TEST_ASSERT_TRUE(std::ferror(file) > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!= 0
(explicit or implied) - it isn't specified as returning positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
TestFile<FS>::resetFunctionCallHistory(); | ||
ret = std::fgetc(file); | ||
#ifndef __ICCARM__ | ||
// IAR optimize reads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the thing that the IAR unbuffered thing was working around, so it now behaves wrongly? Or does it break either way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just behaves differently than other, but we could normalize read part and only test the first read against retargeting layer read call and remove code from ifdefs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it making a single read(N) call rather than multiple read(1)? So it must be buffering? Which is what I think the original #770 was working around.
I would say that is invalid behaviour, but may not be too fatal if read(N) doesn't actually wait for N characters, but returns just what it has. The #770 fix may have been prompted by Stream::read(N) doing that wrongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That kind of makes sense. Serial::read(N) waits for N characters, so Serial::scanf would have jammed. But the built-in stdin "read" always waited for 1 and returned 1, so scanf would have worked.
UARTSerial::read(N) now works correctly, returning 1-N bytes as soon as it has anything.
Maybe the set_unbuffered_stream workaround is only really needed when running against a TTY FileHandle with broken read().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not wrong they read 512B at first 'fread' call. Can't check this now due to disabled VPN:(
TestFile<FS>::resetFunctionCallHistory(); | ||
ret = std::fgetc(file); | ||
TEST_ASSERT_TRUE(TestFile<FS>::functionCalled(TestFile<FS>::fnRead)); | ||
TEST_ASSERT_TRUE(std::feof(file) > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, non-zero. Not specified as positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
What IAR EW version is used for the tests? Is there a project I can use for testing/debugging the (incorrect) fwrite behavior in IAR EW? |
IAR EW for ARM: 7.80.4.12495 |
55a78c8
to
a1f2482
Compare
Thanks. 7.80 is old and there are no updates planned. Has this been tested with IAR EW 8.x? Are you using the standard libraries as shipped from IAR, or has there been any modification to them? |
a1f2482
to
4ec4ea4
Compare
not yet
use standard without modification |
Any project available, IAR EW 7.80 or 8, that shows the misbehaving fwrite so that we may test it to make sure we don't have a problem in EW 8? |
main.cpp with test will be enought? |
4ec4ea4
to
b196332
Compare
Preferably something that can be imported into IAR EWARM and run there, on the simulator or real HW. Thanks. |
b196332
to
8e1cfe2
Compare
@TTornblom Code for testing Also prepared repo with generated project files |
I have been looking into the IAR fwrite() loop issue and in the test code I received I notice that write() returns -ENOSPC (-28), which is returned to fflushOne(), which called from fflush(). fflushOne() only accepts _LLIO_ERROR (-1) or 0 as error return from write(), so returning -ENOSPC (-28) is not a valid return value. From http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html:RETURN VALUE
|
I believe that problem occurs on master, but has already been fixed on #5571 - On current master So, which of the tests still fail with #5571? |
@maciejbocianski #5571 is ready, waiting for the tests ! |
The test should pass also with #5571, failing cases are turned off per compiler by ifdefs |
/morph build |
Build : SUCCESSBuild number : 1073 Triggering tests/morph test |
Test : SUCCESSBuild number : 881 |
Exporter Build : SUCCESSBuild number : 751 |
@kjbracey-arm Happy with this going to master, and addressing issues that were found separately? I imagine this goes in, 5571 retested with master + this PR . Then we can focus on outstanding issues with the tests. |
Yes. |
Tested on top of #5571 and the problem with infinite loop is not present
There is still problem with |
ARM C does the same thing. I'd say fwrite returning something >= 0 and < requested is sufficient to pass. I can't think of a real use case for knowing the exact number - applications will just stop noting failure when they see < requested. Didn't we have a case where it was returning "actual + 1" or "requested - number"? Those would be problematic. |
Ok, I thought the issue was that fwrite would loop during the test. I have verified that fwrite does indeed return 0 when it fails writing all the data, even if it has written some of it. I will file a bug for this. If returning 0 in this case is acceptable, I will give it a low priority. |
I believe we did have some worse problems though, if you read back up the comments. Streams being buffered even when they shouldn't have been, and weird behaviour if the #770 "use a 2-byte buffer" workaround for that is active. @maciejbocianski can hopefully re-summarise. |
I have added a comment to #770. If there are still problems with this, please elaborate and I will look into it. |
Description
This PR contains Mbed retargeting systems tests for C library FILE functions
Status
IN DEVELOPMENT
Migrations
NO
Related PRs
#5714