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

Fix for unit test build on Windows machines #8526

Merged
merged 1 commit into from Oct 26, 2018

Conversation

Projects
None yet
6 participants
@michalpasztamobica
Contributor

michalpasztamobica commented Oct 24, 2018

Description

When building unit tests on windows a linker error is reported when linking EthernetInterface test, claiming that EthernetInterface::get_target_default_instance() is undefined. This is caused by MinGW not supporting weak attribute.

The changes address this issue using preprocessor macro to determine that minGW is being used. MinGW will only be used in case unit tests are built on windows, as every other compilation will run a target compiler (either ARM or target GCC) or a Unix GCC (both on Linux and Mac).

I checked that target code builds fine on Windows and that unit tests are compiling and running on Linux.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2018

I assume we might have a problem with other functions as well. So applying this only to EthernetInterface method is short term fix.

This is caused by MinGW not supporting weak attribute.

Is it? Isn't this related how gcc defines attribute weak ? I believe it works for elf or .out . What do we generate with Mingw ? Can you provide reference?

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-ipcore Oct 24, 2018

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 24, 2018

Sorry, you are right, it is the Windows executable that does not support weak, here's the reference:

"Weak symbols are supported for ELF targets, and also for a.out targets when using the GNU assembler and linker."

I agree that the weak attribute is heavily used in mbedOS source code, but we usually compile to target binaries, which are not Windows executables. The only situation I came across when we compile into a Windows executable is when we we try to run unit tests on Windows. I fixed the only compilation error that was being thrown in this situation.

I can think of a more general change inside platform/mbed_toolchain.h:

#ifndef MBED_WEAK
#if defined(__ICCARM__)
#define MBED_WEAK __weak
+ #elif defined(__MINGW32__)
+ #define MBED_WEAK 
#else
#define MBED_WEAK __attribute__((weak))
#endif
#endif

On one hand it makes sense, but on the other, maybe it is wise to stick to the less-intrusive fix?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2018

On one hand it makes sense, but on the other, maybe it is wise to stick to the less-intrusive fix?

This might seem now, but once we have more unittest coverage - more weak problems - we would end up having this exception in more files. I would rather have it in one place with a proper commit.

``

  • #elif defined(MINGW32)
  • #define MBED_WEAK
    ``

Looks fine to me.

I could not find anything that could support weak linkage in MinGW thus unit test should have this as limitation - all symbols are strongly linked (at least for windows).

@ARMmbed/mbed-os-core Please review

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Oct 24, 2018

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Oct 24, 2018

This might seem now, but once we have more unittest coverage - more weak problems - we would end up having this exception in more files.

I would prefer a generic solution in platform/mbed_toolchain.h: then per file basis. A quick grep in mbed-os\features\netsocket folder itself showed me multiple instances of MBED_WEAK

cellular/generic_modem_driver/OnboardCellularInterface.cpp:67:MBED_WEAK CellularBase *CellularBase::get_target_default_instance()
cellular/generic_modem_driver/OnboardCellularInterface.cpp:74:MBED_WEAK CellularBase *CellularBase::get_target_default_instance()
emac-drivers/README.md:86:    MBED_WEAK EMAC &EMAC::get_default_instance()
emac-drivers/TARGET_Freescale_EMAC/kinetis_emac.cpp:598:MBED_WEAK EMAC &EMAC::get_default_instance() {
emac-drivers/TARGET_NUVOTON_EMAC/numaker_emac.cpp:396:MBED_WEAK EMAC &EMAC::get_default_instance() {
emac-drivers/TARGET_NXP_EMAC/TARGET_LPC546XX/lpc546xx_emac.cpp:592:MBED_WEAK EMAC &EMAC::get_default_instance() {
emac-drivers/TARGET_NXP_EMAC/TARGET_LPCTarget/lpc17_emac.cpp:968:MBED_WEAK EMAC &EMAC::get_default_instance() {
emac-drivers/TARGET_RZ_A1_EMAC/rza1_emac.cpp:11:MBED_WEAK EMAC &EMAC::get_default_instance() {
emac-drivers/TARGET_Silicon_Labs/sl_emac.cpp:708:MBED_WEAK EMAC &EMAC::get_default_instance() {
emac-drivers/TARGET_STM_EMAC/stm32xx_emac.cpp:573:MBED_WEAK EMAC &EMAC::get_default_instance()
EthernetInterface.cpp:24:MBED_WEAK EthInterface *EthInterface::get_target_default_instance()
EthernetInterface.cpp:30:MBED_WEAK EthInterface *EthInterface::get_target_default_instance()
NetworkInterfaceDefaults.cpp:28:MBED_WEAK EthInterface *EthInterface::get_default_instance()
NetworkInterfaceDefaults.cpp:33:MBED_WEAK WiFiInterface *WiFiInterface::get_default_instance()
NetworkInterfaceDefaults.cpp:38:MBED_WEAK MeshInterface *MeshInterface::get_default_instance()
NetworkInterfaceDefaults.cpp:43:MBED_WEAK CellularBase *CellularBase::get_default_instance()
NetworkInterfaceDefaults.cpp:54:MBED_WEAK WiFiInterface *WiFiInterface::get_target_default_instance()
NetworkInterfaceDefaults.cpp:60:MBED_WEAK NetworkInterface *NetworkInterface::get_default_instance()
NetworkInterfaceDefaults.cpp:74:MBED_WEAK NetworkInterface *NetworkInterface::get_target_default_instance()
NetworkInterfaceDefaults.cpp:79:MBED_WEAK NetworkInterface *NetworkInterface::get_target_default_instance()
NetworkInterfaceDefaults.cpp:106:MBED_WEAK NetworkInterface *NetworkInterface::get_target_default_instance()
NetworkInterfaceDefaults.cpp:111:MBED_WEAK NetworkInterface *NetworkInterface::get_target_default_instance()
NetworkInterfaceDefaults.cpp:141:MBED_WEAK NetworkInterface *NetworkInterface::get_target_default_instance()
@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 25, 2018

I too would like to see a more generic fix, but I'm wondering something.

@michalpasztamobica @SeppoTakalo Is this PR needed to unblock some new things in CI, or something similar?

I agree that this seems like there will be more issues down the road once more unittest are added, but I'm wondering if we aren't doing some premature optimization in suggesting to solve the problem in a generic manner.

Thinking out loud. Would bringing this PR in, along with opening a new issue to find a more generic solution be a good path forward for all opinion-holders?

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Oct 25, 2018

If issue is blocking CI, then yes I am fine with merging the PR for now with new issue

Fix for unit test build on Windows machines
In case MINGW is detected - define MBED_WEAK to be empty, as Windows
executables cannot handle the weak attribute.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:windows_unittest_compilation_fix branch from 582345b to 0b3593f Oct 25, 2018

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 25, 2018

I tested that the change in platform/mbed_toolchain.h works just as good as my previous, more scattered yet less intrusive fix. I also checked that the target compilation is unaffected (including no crashes at runtime) and that unittests compile and run fine on Linux.
The fix is much neater now, in my opinion.

This issue was not blocking the whole CI. It was blocking unittests on Windows. I guess most developers use Unix, but even those who use Windows should run a unittest suite before committing anything, so we'd better have it ready for them to use.

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 25, 2018

@SeppoTakalo , please review.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2018

Someone from @ARMmbed/mbed-os-core please review

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2018

@michalpasztamobica One thing - is there anywhere we should document this? I assume with unittest you would not rely on weakly linkage, just making sure this can be captured in the docs somewhere or is that clear to everyone?

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 25, 2018

Nothing comes to my mind really... @SeppoTakalo - is there any relevant documentation where we should take a note of this change?

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 25, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 26, 2018

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 26, 2018

This probably becomes evident for whoever is going to write the Unittest.

Unittest writing requires so much more knowledge than what has been written in the documentation, so adding this causes more questions than what it answers. So I'm OK for leaving it unmentioned.

@cmonr cmonr merged commit c25f156 into ARMmbed:master Oct 26, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 677 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10283 cycles (+31 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment