Include xtensa-types.h header file to support the ACP_7_0 platform's build. #9413
Conversation
This file contains 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
saisurya-ch
requested review from
kv2019i,
marc-hb,
bhiregou,
bhiregoudar and
sunilkumardommati
saisurya-ch
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej and
dbaluta
as code owners
|
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 |
|
Can we just do the ugly @dbaluta, @LaurentiuM1234 the |
I have mentioned the same here that the existing platforms don't support this inclusion. |
Please observe that no other platform's Even dropping @paulstelian97 Can we make the line 48 |
saisurya-ch
force-pushed
the
vangoogh_docker
branch
from
4465822
to
b43cd0e
Compare
|
I wonder if using |
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
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.
Even under the macro |
|
When the code in file * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
#ifndef XTENSA_CONFIG_CORE_H
#define XTENSA_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
#ifndef UINT32_C
#define UINT32_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: |
|
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 |
|
Please try this if you haven't yet: #if defined(__ASSEMBLY__)
#ifndef UINT32_C
#define UINT32_C(x) x
#endif
#else
#ifndef __UINT32_C
#define __UINT32_C(x) x ## U
#endif
#ifndef UINT32_C
#define UINT32_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. |
Below is the console output with the given code. |
|
My bad, can you try this instead: |
works fine with @paulstelian97 Hope removing the condition |
saisurya-ch
force-pushed
the
vangoogh_docker
branch
from
b43cd0e
to
8172484
Compare
marc-hb
reviewed
Sep 4, 2024
| @@ -42,17 +42,22 @@ | |||
| * building SOF with Zephyr and XtensaTools. | |||
| */ | |||
|
|
|||
| #if defined(__ZEPHYR__) | |||
| #if defined(_ASMLANGUAGE) | |||
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...
saisurya-ch
force-pushed
the
vangoogh_docker
branch
2 times, most recently
from
7a7fb57
to
8a4e96b
Compare
dcpleung
approved these changes
Sep 4, 2024
marc-hb
approved these changes
Sep 4, 2024
|
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
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. | |||
There was a problem hiding this comment.
Need to update that comment
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."
There was a problem hiding this comment.
Also, it's not just Zephyr now. And it never was ACP7-specific.
|
(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>
saisurya-ch
force-pushed
the
vangoogh_docker
branch
from
8a4e96b
to
345c95d
Compare
|
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>
@saisurya-ch can you please try my brand new #9442 instead with your toolchain? It includes |
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.