Uh oh!
There was an error while loading. Please reload this page.
Include xtensa-types.h header file to support the ACP_7_0 platform's build. - #9413
Closed
saisurya-ch wants to merge 2 commits into
Closed
Include xtensa-types.h header file to support the ACP_7_0 platform's build.#9413saisurya-ch wants to merge 2 commits into
saisurya-ch wants to merge 2 commits into
Conversation
saisurya-ch
requested review from
bhiregou, bhiregoudar, kv2019i, marc-hb and sunilkumardommatiAugust 28, 2024 09:40
saisurya-ch
requested review from
dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart
as code ownersAugust 28, 2024 09:40
Collaborator
This is a tricky one. Previous discussions: "No such file" https://github.com/thesofproject/sof/actions/runs/10594196451/job/29357272028?pr=9413 https://sof-ci.01.org/sofpr/PR9413/build7462/build/index.html |
Collaborator
Can we just do the ugly @dbaluta, @LaurentiuM1234 the |
MemberAuthor
I have mentioned the same here that the existing platforms don't support this inclusion. |
MemberAuthor
Please observe that no other platform's Even dropping @paulstelian97 Can we make the line 48 |
dcpleung
commented
Aug 29, 2024
Contributor
I wonder if using |
Collaborator
This looks just wrong.
Yes: as defined in the C99 standard.
Please share more about this. We're discussing a PR to fix an issue that most people don't know about: this is what makes this fix almost impossible to review. Please share the small bit of code that does not compile and the complete error trace. Is it not C code but assembly as just guessed by @dcpleung ? If yes then his solution of replacing One example: |
andyross
requested changes
Sep 3, 2024
andyross
left a comment
Contributor
There was a problem hiding this comment.
Driving by: are we sure this isn't just a missing stdint.h somewhere?
Basically: UINT32_C is an API provided by stdint.h. That's provided by the toolchain, not SOF. And it seems from the comment that the Cadence headers in the same toolchain are using it without including the relevant header. That's likely a toolchain bug (and as mentioned, details on the failure would be helpful). But if it works on other platforms and not this one it's probably because the platform layers include stdint.h higher up the include tree, which would IMHO be a better fix.
-1 for now pending details that explain why it's not as simple as I think it should be.
saisurya-ch
commented
Sep 3, 2024
MemberAuthor
Even under the macro |
MemberAuthor
When the code in file *SOFTWAREORTHEUSEOROTHERDEALINGSINTHESOFTWARE.
*/
#ifndefXTENSA_CONFIG_CORE_H#defineXTENSA_CONFIG_CORE_H/* * This define is used by the new 2023 xt-clang toolchain and, while there are a few definitions * (identical to this one) in various implementations such as newlib, none of them is in use when * building SOF with Zephyr and XtensaTools. */#ifndef__UINT32_C#define__UINT32_C(x) x ## U
#endif#ifndefUINT32_C#defineUINT32_C(x) __UINT32_C(x)
#endif/* CONFIGURATION INDEPENDENT DEFINITIONS: */#ifdef__XTENSA__#include<xtensa/hal.h>#include<xtensa/xtensa-versions.h>#else#include"../hal.h"#include"../xtensa-versions.h"#endif/* CONFIGURATION SPECIFIC DEFINITIONS: */#ifdef__XTENSA__#include<xtensa/config/core-isa.h>#include<xtensa/config/core-matmap.h>#include<xtensa/config/tie.h>#else#include"core-isa.h"#include"core-matmap.h"#include"tie.h"#endif#if defined (_ASMLANGUAGE) || defined (__ASSEMBLER__)
#ifdef__XTENSA__#include<xtensa/config/tie-asm.h>#else#include"tie-asm.h"#endif#endif/*_ASMLANGUAGE or __ASSEMBLER__*/below is the console's error output: Details |
marc-hb
commented
Sep 3, 2024
Collaborator
Thanks, these logs are useful. Next time please So it is a problem when compiling ASSEMBLY, great guess @dcpleung . More specifically: int-vector.S, exc-c-wrapper-handler.S and int-initlevel.S, core-save.S and maybe others. These include
What exactly did you try? Please share your |
marc-hb
commented
Sep 3, 2024
Collaborator
Please try this if you haven't yet: #if defined(__ASSEMBLY__)
#ifndefUINT32_C#defineUINT32_C(x) x
#endif#else#ifndef__UINT32_C#define__UINT32_C(x) x ## U
#endif#ifndefUINT32_C#defineUINT32_C(x) __UINT32_C(x)
#endif#endif// __ASSEMBLY__I am NOT saying this is the "correct" and final solution but if it passes then it will great progress. |
MemberAuthor
Below is the console output with the given code. Details |
marc-hb
commented
Sep 4, 2024
Collaborator
My bad, can you try this instead: |
saisurya-ch
commented
Sep 4, 2024
MemberAuthor
works fine with @paulstelian97 Hope removing the condition |
marc-hb
reviewed
Sep 4, 2024
| */ | ||
| #if defined(__ZEPHYR__) | ||
| #if defined(_ASMLANGUAGE) |
Collaborator
There was a problem hiding this comment.
Suggested change
| #if defined(_ASMLANGUAGE) | |
| #if defined(_ASMLANGUAGE)|| defined(__ASSEMBLER__) |
like in zephyrproject-rtos/zephyr@f7538f0
That Zephyr commit is from 2017 so it has stood the test of time.
EDIT: just realized the same condition is already used in the same file...
dcpleung
approved these changes
Sep 4, 2024
marc-hb
approved these changes
Sep 4, 2024
Collaborator
Still does not work because Now we finally understand why @paulstelian97 used https://github.com/thesofproject/sof/actions/runs/10706803167/job/29685346073?pr=9413 exc-sethandler.c#include<xtensa/config/core.h>#include"xtos-internal.h" |
marc-hb
requested changes
Sep 4, 2024
marc-hb
left a comment
Collaborator
There was a problem hiding this comment.
Can you try changing the include order in exc-sethandler.c in a first, separate commit?
It seems to be the only file failing now?
Including more generic headers before more specific ones cannot hurt.
--- a/src/arch/xtensa/xtos/exc-sethandler.c+++ b/src/arch/xtensa/xtos/exc-sethandler.c@@ -24,8 +24,8 @@
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
-#include <xtensa/config/core.h>
#include "xtos-internal.h"
+#include <xtensa/config/core.h>
#if XCHAL_HAVE_EXCEPTIONS| @@ -42,17 +42,22 @@ | |||
| * building SOF with Zephyr and XtensaTools. | |||
Contributor
There was a problem hiding this comment.
Can we rephrase? IMHO this update just makes it more confusing than it already was, suggest something like:
"xt-clang platform headers use this (ISO C) macro to define constants, which appends a "U" suffix to force the type of the literal constant. But those headers end up getting included in assembly files which don't support that syntax. Fake the API for assembly builds here in the SOF toolchain layer."
Collaborator
There was a problem hiding this comment.
Also, it's not just Zephyr now. And it never was ACP7-specific.
andyross
commented
Sep 4, 2024
Contributor
(oops, was dismissing my own -1 because it's been explained now as an assembler error that doesn't understand the same literal syntax... ...and misclicked and dismissed @marc-hb's instead! Obviously I can't replace that, but please don't merge until he has a chance to replace the review) |
Invert the header file inclusion order. Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
Add UINT32_C definition required for core file of platform ACP_7_0. Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
marc-hb
commented
Sep 5, 2024
Collaborator
Still failing in interrupt.h https://github.com/thesofproject/sof/actions/runs/10720712046/job/29727555760?pr=9413 Again, changing the include order fixes it for me, see below. Please test locally with Please take the time to write good and accurate commit messages and comments. This is brittle and will likely fail again. I lost track sorry: are you sure you cannot just --- sof/src/arch/xtensa/include/arch/drivers/interrupt.h+++ sof/src/arch/xtensa/include/arch/drivers/interrupt.h@@ -10,11 +10,12 @@
#ifndef __ARCH_DRIVERS_INTERRUPT_H__
#define __ARCH_DRIVERS_INTERRUPT_H__
-#include <xtensa/hal.h>-#include <xtensa/xtruntime.h>
#include <stddef.h>
#include <stdint.h>
+#include <xtensa/hal.h>+#include <xtensa/xtruntime.h>+ |
marc-hb added a commit
to marc-hb/sof
that referenced
this pull request
Sep 5, 2024
xt-clang uses UINT32_C() without importing it. Commit ef38a0c ("arch: xtensa: core.h: Add define for UINT32_C") added it but only for __ZEPHYR__, add it unconditionally. Provide an alternative, assembly-compatible definition apparently needed by XTOS ACP 7.0 compiled with xt-clang. For C code, simply include <stdint.h> Longer and convoluted story in thesofproject#9413. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
commented
Sep 5, 2024
Collaborator
@saisurya-ch can you please try my brand new #9442 instead with your toolchain? It includes |
MemberAuthor
marc-hb
commented
Sep 5, 2024
Collaborator
Awesome! can you please approve it? |
kv2019i pushed a commit
that referenced
this pull request
Sep 6, 2024
xt-clang uses UINT32_C() without importing it. Commit ef38a0c ("arch: xtensa: core.h: Add define for UINT32_C") added it but only for __ZEPHYR__, add it unconditionally. Provide an alternative, assembly-compatible definition apparently needed by XTOS ACP 7.0 compiled with xt-clang. For C code, simply include <stdint.h> Longer and convoluted story in #9413. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the inclusion of
xtensa-types.hheader file to accommodate the definition of macro UINT32_C which is required by the core filetie.hin platform ACP_7_0. ACP7_0 uses the new xt-clang compiler.