Skip to content

Makefile.{base,include}: Fix linking for C++ code in external modules#14481

Merged
benpicco merged 5 commits intoRIOT-OS:masterfrom
maribu:cpp-fix-linking
Jul 28, 2020
Merged

Makefile.{base,include}: Fix linking for C++ code in external modules#14481
benpicco merged 5 commits intoRIOT-OS:masterfrom
maribu:cpp-fix-linking

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Jul 9, 2020

Contribution description

Internal modules are not allowed to use C++ code, as some platforms (might) only support C. This restriction should however not apply to external modules. If an application uses no C++ code but uses an external module using C++ code, linking is still done using $(CC) instead of $(CXX). This PR

  • Use $(CXX) for linking if the feature cpp is used, rather than searching the application for C++ sources
  • Abort builds when C++ code is detected without feature cpp being used, so that a more helpful error message is presented instead of a linking failure

Testing procedure

The example given in issue #14466 should now error with a helpful error message instead of a linking issue. Add FEATURES_REQUIRED += cpp should fix the linking issue, even if no C++ source code is present in the application.

Issues/PRs references

Fixes: #14466

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: C++ Area: C++ wrapper Area: build system Area: Build system Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Jul 9, 2020
@maribu maribu requested a review from miri64 July 9, 2020 19:26
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jul 10, 2020

Note: This changes behavior slightly.

If in master an application used C++ code, it will trigger the following behavior

  • C++ code will be compiled
  • $(CXX) will be used for linking
  • Feature cpp will not be automatically used.
    • Thus, builds for platforms not supporting C++ will still succeed, if a C++ compiler is available
    • Building on platforms that perform the constructor call of statically allocated class instances only with feature cpp being used will result in broken binaries, without any warning

With this PR the behavior is as follows:

  • The build will fail if C++ code is detected, but feature cpp is not used

In some cases this could be seen as a regression, as previously dropping C++ code in the application without FEATURES_REQUIRED += cpp in the app's Makefile did actually work in some cases. (E.g. when no statically allocated class instances with a constructor are present.) But the "migration cost" of just having to add FEATURES_REQUIRED += cpp is not too high. Also, I would argue that not using FEATURES_REQUIRED += cpp previously was not correct, too. It just happened to work in specific cases by chance.

Auto-magically detecting C++ code and using the feature cpp is not possible in a consistent way, as the source files are only collected after the dependencies and features have been resolved. (I think for the application's source files this is not true, but for external modules and packages it is.) I would prefer to not add the auto-magical detection for consistent behavior between application and module source files.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 10, 2020
benpicco
benpicco reviewed Jul 10, 2020
View reviewed changes
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jul 10, 2020

Let's see how many apps will complain about this. If all apps build without problem, I would say we merge this into the release before feature freeze, otherwise we don't. I see this more as an enhancement BTW than a bug fix, as it adds a warning about wrong usage rather than fixing something in the code :-).

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jul 10, 2020

Nope, lots of boards and applications failing, so sorry, postponing for next release.

@maribu maribu requested a review from gschorcht as a code owner July 10, 2020 14:29
aabadie
aabadie reviewed Jul 10, 2020
View reviewed changes
@maribu maribu removed the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jul 10, 2020
gschorcht
gschorcht reviewed Jul 12, 2020
View reviewed changes
@maribu maribu mentioned this pull request Jul 12, 2020
Merged
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jul 23, 2020

Please squash!

maribu added 5 commits July 23, 2020 20:18
@maribu
Makefile.include: Link with $(CXX) for C++ code
267c8e1 
When mixing C and C++ code, $(CXX) has to be used for linking. Prior to this
commit, the build system automatically uses $(CXX) if the application contains
C++ source code. However, if C++ is used in an module only, $(CC) is still
used. This has not let to problems, as internal modules must be written in C.
For external modules this restriction does not apply.

This commit checks if the cpp feature is used. In that case, $(CXX) is used
for linking over $(CC). This way external modules may use C++ code.
@maribu
Makefile.base: Error for C++ code w/w cpp feature
c1ad435 
If external modules use C++ code and forget to require the `cpp` feature,
linking will still take places using $(CC). This commit adds a check if C++ code
was detected without the feature `cpp` being used, now the build is aborted
with a helpful error message rather than a linker error.
@maribu
makefiles/vars.inc.mk: Export FEATURES_USED
057aae1 
This is needed so that features used can be reliably be accessed.
@maribu
cpu/esp_common: Allows use feature cpp
f8b7c89 
The vendor code uses C++ code, thus, C++ support needs to be enabled in any
case.
@maribu
sys/arduino: Require feature cpp
0338070 
Arduino code requires C++ support
@maribu maribu force-pushed the cpp-fix-linking branch from 2b11d87 to 0338070 Compare July 23, 2020 18:18
@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 23, 2020
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jul 27, 2020

This is no longer waiting on AVR gaining C++ support :-)

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 27, 2020
benpicco
benpicco approved these changes Jul 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Murdock is happy too.

@benpicco benpicco merged commit 403b6b1 into RIOT-OS:master Jul 28, 2020
@maribu maribu deleted the cpp-fix-linking branch August 23, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie left review comments

@gschorcht gschorcht gschorcht left review comments

@benpicco benpicco benpicco approved these changes

@miri64 miri64 Awaiting requested review from miri64

+1 more reviewer

@sqrtroot sqrtroot sqrtroot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: C++ Area: C++ wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Linking C++

7 participants

@maribu @miri64 @benpicco @aabadie @sqrtroot @gschorcht @leandrolanzieri