Fix: Return the correct std I/O device handle for Microlib in retarget code #12758

hugueskamba · 2020-04-03T18:16:53Z

Summary of changes

Return the correct filehandle based on the mode requested. The mode is used
as the pathname is always the default one (":tt") for Microlib. The
previous implementation relied on three successive calls to open the std
I/O device handles, this was not the case.

Impact of changes

It is now possible to print to the console when "platform.stdio-minimal-console-only" is set to true and "target.c_lib" is set to "small" when the binary is built with the ARM toolchain.

Migration actions required

N/A

Documentation

N/A

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@evedon @kjbracey-arm @LDong-Arm

ciarmcom · 2020-04-03T19:00:36Z

@hugueskamba, thank you for your changes.
@kjbracey-arm @LDong-Arm @evedon @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2020-04-06T07:03:42Z

You say what the change is in the description, but not why you're doing it. What is microlib passing? Is it documented?

kjbracey · 2020-04-06T07:05:56Z

platform/source/mbed_retarget.cpp

@@ -756,11 +756,13 @@ extern "C" int PREFIX(_write)(FILEHANDLE fh, const unsigned char *buffer, unsign
 extern "C" ssize_t write(int fildes, const void *buf, size_t length)
 {
 #if MBED_CONF_PLATFORM_STDIO_MINIMAL_CONSOLE_ONLY
+// Microlib does not pass STDOUT_FILENO or STDERR_FILENO when _sys_write is called.
+#if !defined(__MICROLIB)


If this is about microlib passing weird things to _sys_write, then the logical fix is to put the workaround in _sys_write. Make it call write with STDOUT_FILENO when passed whatever the bad thing is. You don't want to disable error checking for other users calling write.

Does microlib support files in general?

microlib first opens :tt for which our open() in mbed_retarget returns 0 (stdin):

mbed-os/platform/source/mbed_retarget.cpp

Lines 555 to 563 in 83c1b34

extern "C" FILEHANDLE PREFIX(_open)(const char *name, int openflags)

{

#if defined(__MICROLIB)

// Before version 5.03, we were using a patched version of microlib with proper names

// This is the workaround that the microlib author suggested us

static int n = 0;

if (std::strcmp(name, ":tt") == 0 && n < 3) {

return n++;

}

The open() code suggests :tt should (?) be opened three times (once for in, out, err each) but debugger shows it's only opened once. This might be related to why write(), which is run after open(), gets called with 0 file descriptor instead of 1 (stdout)? @kjbracey-arm

...then the logical fix is to put the workaround in _sys_write.

The current change seemed more logical to me. If the postman is scared of dogs and you have a dog to protect your home, you keep your dog away from the door when the postman arrives. You don't ask for a new postman...🌝.

You don't want to disable error checking for other users calling write.

How will other users be affected since the check is present for everything except Microlib (#if !defined(__MICROLIB))?

write is a public function. People can use it for their own files directly (open/read/close), regardless of any issues in microlib. That's what I meant by "other users" - other users of write in a system using microlib.

In the minimal console case, you do want to trap someone doing something weird - given the above call sequence you want to give an error when given the "-1" handle that open gave you when trying to open a file, not send the stuff that should have gone to file to the console. That check shouldn't be removed because of an issue in one user.

At a minimum you'd want to at least ensure it was a valid TTY handle of some sort - maybe accept <= STDERR_FILENO if the numbers from microlib are fuzzy.

Now, LDong's description looks on the point - something weird is happening with the way microlib opens our streams. Is it that microlib doesn't actually open 3 in sequence like the code assumes? Is it opening on-demand - first call to each of stdin/stdout/stderr?

Does it at least open twice, so we could look at the mode and return either STDIN_FILENO or STDOUT_FILENO?

At a minimum you'd want to at least ensure it was a valid TTY handle of some sort - maybe accept <= STDERR_FILENO if the numbers from microlib are fuzzy.

This is what mbed_file_handle() does (when not using minimal-console):

mbed-os/platform/source/mbed_retarget.cpp

Lines 371 to 386 in 83c1b34

/* Deal with the fact C library may not _open descriptors 0, 1, 2 - auto bind */

FileHandle *mbed_file_handle(int fd)

{

#if !MBED_CONF_PLATFORM_STDIO_MINIMAL_CONSOLE_ONLY

if (fd >= RETARGET_OPEN_MAX) {

return NULL;

}

FileHandle *fh = filehandles[fd];

if (fh == FILE_HANDLE_RESERVED && fd < 3) {

filehandles[fd] = fh = get_console(fd);

}

return fh;

#else

return nullptr;

#endif // !MBED_CONF_PLATFORM_STDIO_MINIMAL_CONSOLE_ONLY

}

when the file descriptor is < 3, it just uses get_console() which returns the default console in most cases. (Also the comment there suggests descriptors 0,1,2 indeed may not be open by the C lib.)

There are a couple of paths into all this code. You are allowed to have an mbed_override_console that sends stdin, stdout and stderr to 3 separate FileHandle devices - that's probably the only place we really care. If mbed_override_console isn't added by the app or target, then all 3 do go to the same device anyway.

Having a restriction that mbed_override_console doesn't allow different streams for microlib because it doesn't properly distinguish wouldn't be a huge deal.

But as I said in my previous comment - maybe it's only not distinguishing because we're not trying. Those :tt previously would have been us knowing what name it was going to give us regardless. But maybe it's now actually obeying them, and we could change them.

_sys_open() is called once with name: ":tt" and openflags: 4 (O_RDONLY | O_NONBLOCK) . Does it make sense to return STDOUT_FILENO with these values...🤔

4 is OPEN_W for the ARM C library (see ARMCompiler6.13/include/rt_sys.h):

/* * Open a file. May return -1 if the file failed to open. */ extern FILEHANDLE _sys_open(const char * /*name*/, int /*openmode*/); /* * openmode is a bitmap, whose bits are given below. They * correspond directly to the ANSI mode specification. */ #define OPEN_R 0 #define OPEN_W 4 #define OPEN_A 8 #define OPEN_B 1 #define OPEN_PLUS 2 /* * These names should be special strings which will be recognised * by _sys_open and will cause it to return the standard I/O * handles, instead of opening a real file. */ extern const char __stdin_name[]; extern const char __stdout_name[]; extern const char __stderr_name[];

That may be because your app is only using stdout though.

What if you use stdin, or stderr, either before or after?

hugueskamba · 2020-04-06T09:27:49Z

You say what the change is in the description, but not why you're doing it. What is microlib passing? Is it documented?

The description has been updated. Microlib passes the value 0 for fh.

kjbracey · 2020-04-06T10:18:30Z

The description has been updated. Microlib passes the value 0 for fh.

Can you probe this a little more. What does it do if you use both stdin and stdout? Can you trace the calls to _sys_open with modes?

Hang on. Do we know that the __stdin_name thing still doesn't work with microlib? It seems like the code was written on the assumption that microlib doesn't support us defining __stdin_name to give them distinct names. It may be worth rechecking that - flip that switch and see if microlib passes those names back to us, or is it still hardcoding :tt? Given talk of a "patched" microlib, maybe that patch made its way upstream for ARM Compiler 6.

LDong-Arm · 2020-04-06T10:54:57Z

or is it still hardcoding :tt? Given talk of a "patched" microlib, maybe that patch made its way upstream for ARM Compiler 6.

Yes, still :tt.

For this bit, my understanding is that the old microlib has the patch to use /stdout, etc. and the new one does not - :tt is used, as the debugger confirms.

mbed-os/platform/source/mbed_retarget.cpp

Lines 100 to 106 in 978db96

    
           #if defined(__MICROLIB) && (__ARMCC_VERSION>5030000) 
        
           // Before version 5.03, we were using a patched version of microlib with proper names 
        
           extern const char __stdin_name[]  = ":tt"; 
        
           extern const char __stdout_name[] = ":tt"; 
        
           extern const char __stderr_name[] = ":tt"; 
        
           #else

(See which way round the version macro is applied.)
The above is from 5.15. The version macro was dropped during the removal ARMC5 support, so we lost the info on master...

kjbracey · 2020-04-06T10:57:28Z

It's worth rechecking though - that's really old code. We were working from the ARMC5 maintenance branch, which probably wasn't getting any functional changes like that. It's quite conceivable that the microlib shipped with ARM Compiler 6 may have had this improved.

(My reading is that early Mbed was using a custom microlib build, but later switched to standard ARM C 5 which no longer had been tweaked).

I can't find anything specifically in the ARM C 6 manual saying that __stdin_name doesn't work with microlib, but that's far from conclusive.

LDong-Arm · 2020-04-06T11:01:56Z

@kjbracey-arm Ah finally got your point.

@hugueskamba Maybe we could try to drop the '#if' part of

mbed-os/platform/source/mbed_retarget.cpp

Lines 100 to 106 in 978db96

    
           #if defined(__MICROLIB) && (__ARMCC_VERSION>5030000) 
        
           // Before version 5.03, we were using a patched version of microlib with proper names 
        
           extern const char __stdin_name[]  = ":tt"; 
        
           extern const char __stdout_name[] = ":tt"; 
        
           extern const char __stderr_name[] = ":tt"; 
        
           #else

to see if microlib actually maps the name to /stdout, etc. which could solve the problem altogether if it works?

hugueskamba · 2020-04-06T12:17:45Z

@kjbracey-arm Ah finally got your point.

@hugueskamba Maybe we could try to drop the '#if' part of

mbed-os/platform/source/mbed_retarget.cpp

Lines 100 to 106 in 978db96

#if defined(__MICROLIB) && (__ARMCC_VERSION>5030000)

// Before version 5.03, we were using a patched version of microlib with proper names

extern const char __stdin_name[] = ":tt";

extern const char __stdout_name[] = ":tt";

extern const char __stderr_name[] = ":tt";

#else

to see if microlib actually maps the name to /stdout, etc. which could solve the problem altogether if it works?

@LDong-Arm your suggestion does not work as _sys_open() is still called with ":tt". I am investigating the best way to solve the issue with returning the correct filehandle for Microlib.

hugueskamba · 2020-04-06T12:49:03Z

@kjbracey-arm
As the __stdX_name variables (still) cannot be re-defined with Microlib. We will have to use the mode.
I came across the following: https://developer.arm.com/docs/100863/0300/semihosting-operations/sys_open-0x01

The semihosting extension SH_EXT_STDOUT_STDERR allows the semihosting caller to open separate output streams corresponding to stdout and stderr. This extension is reported using feature byte 0, bit 1. Use SYS_OPEN with the special path name :semihosting-features to access the feature bits. See Semihosting extensions for details.

If this extension is supported, the implementation must support the following additional semantics to SYS_OPEN:

If the special path name :tt is opened with an fopen mode requesting write access (w, wb, w+, or w+b), then this is a request to open stdout.

If the special path name :tt is opened with a mode requesting append access (a, ab, a+, or a+b), then this is a request to open stderr.

LDong-Arm · 2020-04-06T13:10:11Z

@kjbracey-arm
As the __stdX_name variables (still) cannot be re-defined with Microlib. We will have to use the mode.
I came across the following: https://developer.arm.com/docs/100863/0300/semihosting-operations/sys_open-0x01

The semihosting extension SH_EXT_STDOUT_STDERR allows the semihosting caller to open separate output streams corresponding to stdout and stderr. This extension is reported using feature byte 0, bit 1. Use SYS_OPEN with the special path name :semihosting-features to access the feature bits. See Semihosting extensions for details.
If this extension is supported, the implementation must support the following additional semantics to SYS_OPEN:

If the special path name :tt is opened with an fopen mode requesting write access (w, wb, w+, or w+b), then this is a request to open stdout.

If the special path name :tt is opened with a mode requesting append access (a, ab, a+, or a+b), then this is a request to open stderr.

Looks sensible

kjbracey · 2020-04-06T13:19:21Z

That stuff is about semihosting specifically, but hopefully microlib will follow the same pattern. Again, you should be able to experiment and check what it does.

hugueskamba · 2020-04-06T13:54:04Z

@kjbracey-arm @LDong-Arm ,
Please review again.
Thanks

platform/source/mbed_retarget.cpp

LDong-Arm

LGTM, thanks for the fix

platform/source/mbed_retarget.cpp

…t code Return the correct filehandle based on the mode requested. The mode is used as the pathname is always the default one (":tt") for Microlib. The previous implementation relied on three successive calls to open the std I/O device handles, this was not the case.

kjbracey

Looks good now.

Only remaining concern is that I wonder why microlib doesn't do the call to mbed_file_handle that the other code does.

Digging through the history, that dates back to the initial import - there were 3 calls to init_serial() there which later got transformed into get_fhc(fileno) then finally mbed_file_handle(fileno).

That was never there for microlib.

I think it doesn't matter any more - I think the calls could be removed, as the init happens the first time mbed_file_handle calls get_console - any call referencing the console can do it.

I think I left them in by caution, assuming that the C library was calling _sys_open on start up, so I thought it might be doing something to ensure pre-main init, but in fact the C libraries call _sys_open on first use themselves anyway - that call may never happen. The console might only ever be used via, eg, write(STDERR_FILENO) in a crash. (And we've had to take care to make sure that init can happen in that context if necessary. It's been problematic).

So no change needed for this PR - there is a historical inconsistency, but I don't think it causes a problem.

LDong-Arm · 2020-04-07T15:09:27Z

Only remaining concern is that I wonder why microlib doesn't do the call to mbed_file_handle that the other code does.

This is only the case when we enable minimal-console (no fancy file system support) - not using mbed_file_handle is intended and has nothing to do with the C lib used. @hugueskamba implemented minimal-console a few months back. And the stdio issue this PR addresses is only visible with minimal console.

0xc0170 · 2020-04-14T09:05:20Z

CI started

mbed-ci · 2020-04-14T11:59:40Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

ciarmcom requested review from evedon, kjbracey, LDong-Arm and a team April 3, 2020 19:00

ciarmcom added the needs: review label Apr 3, 2020

kjbracey reviewed Apr 6, 2020

View reviewed changes

hugueskamba force-pushed the hk_fix_microlib_minimal_console branch from 83c1b34 to 04b333e Compare April 6, 2020 13:29

hugueskamba changed the title ~~Microlib: Fix minimal console printing~~ Microlib: Fix console printing Apr 6, 2020

hugueskamba force-pushed the hk_fix_microlib_minimal_console branch from 04b333e to 4ccc802 Compare April 6, 2020 13:34

hugueskamba changed the title ~~Microlib: Fix console printing~~ Fix: Return the correct std I/O device handle for Microlib in retarget code Apr 6, 2020

kjbracey reviewed Apr 6, 2020

View reviewed changes

platform/source/mbed_retarget.cpp Outdated Show resolved Hide resolved

hugueskamba force-pushed the hk_fix_microlib_minimal_console branch from 4ccc802 to e7b05d5 Compare April 6, 2020 15:07

LDong-Arm approved these changes Apr 6, 2020

View reviewed changes

kjbracey reviewed Apr 7, 2020

View reviewed changes

platform/source/mbed_retarget.cpp Outdated Show resolved Hide resolved

hugueskamba force-pushed the hk_fix_microlib_minimal_console branch from e7b05d5 to eca09b7 Compare April 7, 2020 12:10

LDong-Arm approved these changes Apr 7, 2020

View reviewed changes

kjbracey approved these changes Apr 7, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Apr 7, 2020

0xc0170 added the release-version: 6.0.0-beta-1 label Apr 7, 2020

evedon approved these changes Apr 8, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Apr 14, 2020

0xc0170 merged commit d4da298 into ARMmbed:master Apr 15, 2020

mergify bot removed the ready for merge label Apr 15, 2020

hugueskamba deleted the hk_fix_microlib_minimal_console branch April 15, 2020 08:13

	extern "C" FILEHANDLE PREFIX(_open)(const char *name, int openflags)
	{
	#if defined(__MICROLIB)
	// Before version 5.03, we were using a patched version of microlib with proper names
	// This is the workaround that the microlib author suggested us
	static int n = 0;
	if (std::strcmp(name, ":tt") == 0 && n < 3) {
	return n++;
	}

	/* Deal with the fact C library may not _open descriptors 0, 1, 2 - auto bind */
	FileHandle *mbed_file_handle(int fd)
	{
	#if !MBED_CONF_PLATFORM_STDIO_MINIMAL_CONSOLE_ONLY
	if (fd >= RETARGET_OPEN_MAX) {
	return NULL;
	}
	FileHandle *fh = filehandles[fd];
	if (fh == FILE_HANDLE_RESERVED && fd < 3) {
	filehandles[fd] = fh = get_console(fd);
	}
	return fh;
	#else
	return nullptr;
	#endif // !MBED_CONF_PLATFORM_STDIO_MINIMAL_CONSOLE_ONLY
	}

Fix: Return the correct std I/O device handle for Microlib in retarget code #12758

Fix: Return the correct std I/O device handle for Microlib in retarget code #12758

Uh oh!

Conversation

hugueskamba commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Apr 3, 2020

Uh oh!

kjbracey commented Apr 6, 2020

Uh oh!

kjbracey Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

hugueskamba commented Apr 6, 2020

Uh oh!

kjbracey commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LDong-Arm commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LDong-Arm commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hugueskamba commented Apr 6, 2020

Uh oh!

hugueskamba commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LDong-Arm commented Apr 6, 2020

Uh oh!

kjbracey commented Apr 6, 2020

Uh oh!

hugueskamba commented Apr 6, 2020

Uh oh!

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kjbracey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba commented Apr 3, 2020 •

edited

Loading

kjbracey Apr 6, 2020 •

edited

Loading

LDong-Arm Apr 6, 2020 •

edited

Loading

kjbracey Apr 6, 2020 •

edited

Loading

kjbracey Apr 6, 2020 •

edited

Loading

kjbracey commented Apr 6, 2020 •

edited

Loading

LDong-Arm commented Apr 6, 2020 •

edited

Loading

kjbracey commented Apr 6, 2020 •

edited

Loading

LDong-Arm commented Apr 6, 2020 •

edited

Loading

hugueskamba commented Apr 6, 2020 •

edited

Loading

kjbracey left a comment •

edited

Loading

LDong-Arm commented Apr 7, 2020 •

edited

Loading