Mbed-os: Incorrect inclusion due to header file name clash

[LiyouZhou]

./mbed-os/core/hal/api/critical.h clash with an header file in an external module https://github.com/ARMmbed/atomic-queue/blob/master/source/critical.h.

./mbed-os/core/hal/common/Ticker.cpp includes the wrong file without warning. Fail to find symbols.

This is a common scenario and there should be a strategy in place in mbed tools to avoid these situation from happening.

[0xc0170]

yes, we could rename it, prefix core_util as API is, to overcome this. As it's already used by some software, we could do a warning in the current critical.h for some period of time to enable users to start using a new one. However , this is just one file, we got more in the code base and renaming does not look as a backward compatible solution :sob:
One would be to use absolute paths that would work outside of mbed lib (mbed library is flatten aka all headers from common/api are in mbed folder - something we can consider changing or start using as mbed/critical.h).

any suggestions welcome.

[LiyouZhou]

I think to solve the problem once and for all. Compile mbed-os into a library, expose only the API headers to outside mbed-os. Properly namespace the API headers, for C as well as CPP.

[bremoran]

To solve this problem, the files in each directory need to be compiled separately, so that local includes always take precedence.

Include order must also be different for each directory. For example, suppose I have four 3rd party libraries: a, b, c, d. a depends on b::foo, and c depends on d::foo. If the include order is wrong, then there will be more faults like this.

[marcuschangarm]

@screamerbg @mjs-arm @sg-

[0xc0170]

To solve this problem, the files in each directory need to be compiled separately, so that local includes always take precedence.

Include order must also be different for each directory. For example, suppose I have four 3rd party libraries: a, b, c, d. a depends on b::foo, and c depends on d::foo. If the include order is wrong, then there will be more faults like this.

How to enforce this in the IDEs? They provide options to change project settings for a virtual folder or a file. that would mean that each directory or a separate module would need to be in its own virtual folder with own settings. I know that IAR and uvision support overriding settings per file/virtual folder, not sure about the rest of IDEs.

[bremoran]

When each module is built separately, it is the responsibility of the module developer to ensure the correct include order. When all modules are built together, it is the responsibility of the build tool to answer this question.

[LiyouZhou]

When you try to support multiple build systems, you cannot expect all of the features of the build systems to match up perfectly. This is not the reason for keep mbed minimal. Just because uvision ignores this problem, does not mean that we should. If mbed offers more features than uvision, how is this a problem?

[jaustin]

@LiyouZhou I don't think it's as simple as that - we want mbed to be as useful to as many people as possible - there's a balance between features and availability of tools - lots of developers like using uvision and if they can't use mbed with it, they might not use mbed at all. I think we need to keep 'core' behaviour working in our main export IDEs and there can be some features/niceities/extra tools that are specific to each IDE.

I think the problem this bug is discussing is large enough that we need to find a solution that works across the tools.

It's worth noting that the behaviour you describe is more-or-less what happens with the online IDE's .bld files at the moment, so not out of the question to extend to other tools.

@sg- and @0xc0170 and @screamerbg - we'll need to work out some mechanisms for this

[ghost]

Header files need to be name spaced to avoid collision, build tools won;t save you. There are are two approaches to name spacing:

1) #include "foo/bar.h"
2) #include "foo_bar.h"

We are already, so far as I can see, already heading down route 1)

[bogdanm]

Header files need to be name spaced to avoid collision, build tools won;t save you

+1.

[bremoran]

@mjs-arm
I don't see how this can save you from incorrect include order in 3rd party modules. This was a failure caused in a local include.

Supposed I have:

module_a/foo.h
module_a/foo.c:

#include "foo.h"

module_b/foo.h
module_b/foo.c:

#include "foo.h"

The build tools must not break this

[ghost]

Third party software integrated into the tree is not exempt from managing their name space pollution.

include "foo.h" is NOT ok for third parties either.

[bremoran]

3rd parties are only subject to this requirement if they explicitly support mbed OS. This will be limiting for our users when they want to incorporate third-party software.

It also creates a maintenance overhead for us when incorporating 3rd party software. A requirement to change the names of 3rd party modules means that we must always have a diff between our version of a module and the upstream.

In the extreme case, this could lead to unexpected behaviour: when a new version of a 3rd party module is pulled into the project and the diff is applied, an include directive could be missed if it is in a file where it did not previously exist. If this does not generate a compilation error (for example due to applying sensible defaults in the including file) unexpected behaviour could result if there is another identically named file on the include path.

[ghost]

@bremoran this issue is not unique to mbed-os. Managing name space is a problem that everyone writing code with the objective of portability needs to face up to. Code that is developed that does not manage its namespace effectively is inherently not portable. The two options I listed above are not mbed-os centric solutions, they are off the shelf standard practice and have been since the dawn of time.

[bremoran]

Writing portable C:
1) Externally facing APIs and files should be namespaced.
2) Internal APIs not marked static should be namespaced if static linking is expected

Include files that are only for local use should not be exposed to the include search path. This also is standard practice since the dawn of C (I won't claim time on this one). For example, see the omnipresent config.h.

[geky]

Hi everyone, this discussion has settled a bit, so I'd like to get a set of naming guidelines in place to give this issue resolution. I'm going to toss out a proposal based on what exists in the current codebase, feel free to criticize and discuss.

---

Modules

First thing to note, even as one codebase, mbed is composed of a set of modules (hal, rtos, nsapi, lwip, nanostack, uvisor...). Each module should have a unique name to avoid naming conflicts with other modules. No other folder in the codebase should share a name with a module to avoid include conflicts.

Files

All includes must have the path from the containing module to properly namespace header files. For example rtos/Thread.h or hal/api/DigitalIn.h. This is the 1st option pointed out by @mjs-arm and matches the include pattern used in mbed 3. Internal module headers must also include this namespace.

Having the build system architected such that internal module headers are correctly included would be nice for the reasons put forward by @bremoran, but this is currently unsupported and I believe this feature is not currently planned. If this is needed we should raise a separate issue against the tools.

Module Header

Currently several modules have a centralized module header (mbed.h, rtos.h, nsapi.h). This helps alleviate issues with unintended header inclusions that can cause subtle problems with perfect backwards compatibility. If the module header is unique across all headers in the codebase, no namespace is needed.

Namespaces

This concept hasn't been discussed much, but we do have an existing pattern in our codebase. Using namespaces allows us to use classes between different modules without cluttering the global namespace. Currently several modules impose namespaces on all of their contained classes with the same name as the module. For C files this namespace is imposed through a common prefix of the same name. For convenience the namespace is unwrapped in the centralized module header.

[ciarmcom]

ARM Internal Ref: IOTMORF-164