Semihosting SYS_READ returns incorrect value on EOF

[donmr]

NOTICE: The issue may be closed without notice when not enough information is provided!

Thank you for giving feedback to the stlink project. Take some time to fill out
check boxes with a X in the following items so developers and other people can try to
to find out what is going on. And add/remove what is appropriate to your problem.

When submitting a feature request, try to reuse the list and add/remove what is appropriate.
Place a X between the brackets [X] to mark the list item.

• Programmer/board type: e.g Stlink/v2-1 on STM32F407-DISC (updated to latest).
• Programmer firmware version: e.g STSW-LINK007 V2J30M21
• Operating system: Linux-Mint 18.3
• Stlink tools version and/or git commit hash: v1.4.0-39-g6db0fc2
• Stlink commandline tool name: st-util
• Target chip (and optional board): STM32F407VGT6U

A as-detailed description possible of the problem with debug output when available.

When SYS_READ is called at the end of a file, it is returning a negative value (-len). This does not match the ARM spec and confuses the stdio code on the DUT.

I believe the issue is near line 353 of src/gdbserver/semihosting.c

Output:

2018-07-25T07:18:13 DEBUG semihosting.c: Semihosting: read(14, target_addr:0x2001ff60, 16)
2018-07-25T07:18:13 DEBUG semihosting.c: Semihosting: read 0 bytes
2018-07-25T07:18:13 DEBUG common.c: *** stlink_write_mem32 0 bytes to 0x2001ff60
2018-07-25T07:18:13 DEBUG semihosting.c: Semihosting: return -16

Expected/description:

Per ARM spec, return value should be the same as the requested read length.

[donmr]

I believe that changing line 353 from:
*ret -= buffer_len;
To:
*ret = buffer_len - *ret;
Fixes this problem. I don't have an exhaustive test suite, but it works for full, partial, and empty reads.

[xor-gate]

Yeah potential this is a bug at /~https://github.com/texane/stlink/blob/master/src/gdbserver/semihosting.c#L353. Ping @Fabien-Chouteau not sure what is happening here.

[donmr]

Just a side note, I don't like the way this code stores the result of the read() call into *ret because that value is not the proper return value, ever. I think it would be cleaner to use a local variable for that. Then the computation of the return code would be more clear.

[xor-gate]

Thanks for the fix in #727.

[Fabien-Chouteau]

I agree with the change but I think the patch that was merged is fault as well. See my review.