Skip to content

llvm 11 compile fixes#15595

Closed
kaspar030 wants to merge 7 commits intoRIOT-OS:masterfrom
kaspar030:fix-llvm11-issues
Closed

llvm 11 compile fixes#15595
kaspar030 wants to merge 7 commits intoRIOT-OS:masterfrom
kaspar030:fix-llvm11-issues

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Dec 9, 2020

Contribution description

This PR bundles some llvm changes necessary to build with llvm 11.

Testing procedure

Issues/PRs references

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 9, 2020
@kaspar030 kaspar030 mentioned this pull request Dec 9, 2020
Merged
@maribu maribu mentioned this pull request Dec 9, 2020
Merged
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 9, 2020
@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 15, 2020

Maybe just add -Wno-implicit-int-float-conversion to the utensor package to fix this issue?

@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 15, 2020

And maybe -Wno-c99-designator to all of C++ to fix this? I don't see a reason to not use C designators also in C++. They make the code more readable.

@kaspar030 kaspar030 force-pushed the fix-llvm11-issues branch 2 times, most recently from a1cc392 to 60815d2 Compare December 15, 2020 14:19
@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 Jan 17, 2021
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jan 18, 2021

Murdock is not happy yet.

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 18, 2021

Maybe we should split out the thread size configuration into a separate header, so that cpu_conf.h doesn't need to be included via thread.h?

IMO, C++ code should never see any low level headers and we just provide C++ compatibility for the public API. That way, C++ becomes quite a bit less pain in the ass to maintain.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Jan 18, 2021

Maybe we should split out the thread size configuration into a separate header, so that cpu_conf.h doesn't need to be included via thread.h?

Yes! I have a commit somewhere doing that. I'm on holidays this week, will dig out when back.

@maribu
Copy link
Copy Markdown
Member

maribu commented Feb 10, 2021

Maybe we should split out the thread size configuration into a separate header, so that cpu_conf.h doesn't need to be included via thread.h?

Yes! I have a commit somewhere doing that. I'm on holidays this week, will dig out when back.

I now remembered why I suggested this: The issue is that cpu_conf.h is including the vendor files, which are not compatible with clang++. But C++ code needs to include thread.h. So splitting this out would fix the first compilation error posted by murdock. Maybe you could PR that commit or add it here?

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Feb 10, 2021

I now remembered why I suggested this: The issue is that cpu_conf.h is including the vendor files

This has bitten me on multiple occasions already.
A solution would be to split this so that vendor files are only included by cpu specific code, not by common/application code - but this turned out to be a larger yak to shave than I anticipated.

@kaspar030 kaspar030 mentioned this pull request Feb 16, 2021
Merged
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 16, 2021

Maybe you could PR that commit or add it here?

I found that commit: #16024

That PR is only moving the code so far. As @benpicco stated, dropping the cpu_conf.h includes is a bit more work.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 16, 2021

dropping the cpu_conf.h includes is a bit more work.

... like, the inlined thread_yield_higher() in thread_arch.h also depends on cpu_conf.h ...

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Mar 30, 2021

Is there a fix for:

/data/riotbuild/riotbase/cpu/sam0_common/include/vendor/samr21/include/component/ac.h:226:18: error: anonymous bit-field cannot have qualifiers
    __I uint8_t  :2;               /*!< bit:  2.. 3  Reserved                           */
                 ^

@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 30, 2021

Is there a fix for:

/data/riotbuild/riotbase/cpu/sam0_common/include/vendor/samr21/include/component/ac.h:226:18: error: anonymous bit-field cannot have qualifiers
    __I uint8_t  :2;               /*!< bit:  2.. 3  Reserved                           */
                 ^

Yes, not including the system headers from C++ :-)

Sadly, this is not so trivial. It would be best if we could avoid including vendor files from "public" header files (directly or indirectly) altogether, so that only the C source in /cpu/ would ever see the vendor files. This would also avoid a lot of name collisions between preprocessor macros with application level C code, which can be a pain in the ass. But as said, this is a bit of work.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
kaspar030 added 4 commits May 31, 2022 18:09
@kaspar030
pkg/lua: build with -Wno-string-plus-int
31dc748
@kaspar030
pkg/micro-ecc: blacklist llvm for all cortexm
4a555f9 
```
error: register allocation failed: maximum depth for recoloring reached. Use -fexhaustive-register-search to skip cutoffs
In file included from /home/kaspar/src/riot/build/pkg/micro-ecc/uECC.c:182:
/home/kaspar/src/riot/build/pkg/micro-ecc/asm_arm.inc:62:9: error: inline assembly requires more registers than available
        ".syntax unified \n\t"
        ^
```
@kaspar030
pkt/utensor: add CXXEXFLAGS += -Wno-implicit-int-float-conversion
b238108
@kaspar030
makefiles/cflags.inc.mk: add CXXEXFLAGS += -Wno-c99-designator
13895e9
@kaspar030 kaspar030 force-pushed the fix-llvm11-issues branch from 60815d2 to 8e8c24a Compare May 31, 2022 16:10
@github-actions github-actions bot added Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: pkg Area: External package ports and removed Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … labels May 31, 2022
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Mar 1, 2024

I'm not going to work on this anymore.

@kaspar030 kaspar030 closed this Mar 1, 2024
@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 1, 2024

Oh, it has been fixed anyway in the meantime

@mguetschow
Copy link
Copy Markdown
Contributor

mguetschow commented Mar 18, 2024

But it seems murdock is still not building some boards with llvm: https://github.com/RIOT-OS/RIOT/blob/master/.murdock#L40

@maribu maribu mentioned this pull request Mar 19, 2024
Merged
@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 19, 2024

But it seems murdock is still not building some boards with llvm: https://github.com/RIOT-OS/RIOT/blob/master/.murdock#L40

It has been re-enabled in 52cf2b4 - I forgot to update the comment above, though. That only a subset of the boards are build with LLVM is intentional to avoid increasing CI time by factor 2. I dropped the obsolete comment and added a bit of doc in #20482

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

Reviewers

No reviews

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: pkg Area: External package ports 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.

6 participants

@kaspar030 @maribu @benpicco @fjmolinas @mguetschow @MrKevinWeiss