Skip to content

logger: Make CC_DESC string length indivisible by four - #2647

Merged
lgirdwood merged 1 commit into
thesofproject:masterfrom
ktrzcinx:logger-quick-fix
Mar 30, 2020
Merged

logger: Make CC_DESC string length indivisible by four#2647
lgirdwood merged 1 commit into
thesofproject:masterfrom
ktrzcinx:logger-quick-fix

Conversation

@ktrzcinx

@ktrzcinxktrzcinx commented Mar 27, 2020

Copy link
Copy Markdown
Member

This patch should fix logger without ABI change.

Signed-off-by: Karol Trzcinski karolx.trzcinski@linux.intel.com

@paulstelian97

paulstelian97 commented Mar 27, 2020

Copy link
Copy Markdown
Collaborator

Why do we want it to not be divisible by 4? If it is divisible it would lack a NUL terminator or something? (or have an off-by-one calculation of size)

@jajanusz

jajanusz commented Mar 27, 2020

Copy link
Copy Markdown
Contributor

W/A for issue that should be finally solved by #2522, but is taking too long to get merged and makes logger unusable in CI for weeks.

@jajanusz

jajanusz commented Mar 27, 2020

Copy link
Copy Markdown
Contributor

Now I'm thinking if ranlib may be messing with it (it uses one from gcc instead of xtensa), can you try to repro issue with this change:

diff --git a/scripts/cmake/xtensa-toolchain.cmake b/scripts/cmake/xtensa-toolchain.cmake
index c65caa2a9..bf2d6b362 100644
--- a/scripts/cmake/xtensa-toolchain.cmake+++ b/scripts/cmake/xtensa-toolchain.cmake@@ -56,6 +56,7 @@ endif()
find_program(CMAKE_LD NAMES "${CROSS_COMPILE}ld" PATHS ENV PATH NO_DEFAULT_PATH)
find_program(CMAKE_AR NAMES "${CROSS_COMPILE}ar" PATHS ENV PATH NO_DEFAULT_PATH)
+find_program(CMAKE_RANLIB NAMES "${CROSS_COMPILE}ranlib" PATHS ENV PATH NO_DEFAULT_PATH)
find_program(CMAKE_OBJCOPY NAMES "${CROSS_COMPILE}objcopy" PATHS ENV PATH NO_DEFAULT_PATH)
find_program(CMAKE_OBJDUMP NAMES "${CROSS_COMPILE}objdump" PATHS ENV PATH NO_DEFAULT_PATH)

jajanusz
jajanusz suggested changes Mar 27, 2020
View reviewed changes

@jajanuszjajanusz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to check if can be fixed with ranlib.

dbaluta
dbaluta reviewed Mar 27, 2020
View reviewed changes
Comment threadscripts/cmake/xtensa-toolchain.cmake Outdated

@dbalutadbalutaMar 27, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ktrzcinx I'm totally puzzled by this PR.

Please explain in the commit message what is the ABI change that we are fixing. Also explain WHY this change is needed.

@ktrzcinxktrzcinxMar 27, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

changed

@marc-hbmarc-hbMar 28, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think a few words in the code itself wouldn't hurt either, for now it's just pure maths :-)

@ktrzcinx
logger: Make CC_DESC string length indivisible by four
467e4bf
When variable length array, filled with string will be
placed in sucha manner that null terminator address will
be divisible by four, then it will be lost in output
binary file. It leads to troubles during scanning content
of such a section. Such a problem occur in firmware
and produce logger and FW debug ABI mismatch and it's why
logger output is broken.
After change length of XCC_TOOLS_VERSION to be none of
number four multiplication problem with logger disappear.
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@mmaka1

mmaka1 commented Mar 27, 2020

Copy link
Copy Markdown

Fixes #2507

@marc-hb

marc-hb commented Mar 28, 2020

Copy link
Copy Markdown
Collaborator

Does anyone know why the exact same check-sof-logger test row still fails with a different error? Just curious.

@ktrzcinx

ktrzcinx commented Mar 30, 2020

Copy link
Copy Markdown
MemberAuthor

Does anyone know why the exact same check-sof-logger test row still fails with a different error? Just curious.

It's a good question, @xiulipan do you know the root cause? Locally I can create output logs without any problem but test failed.

@xiulipan

xiulipan commented Mar 30, 2020

Copy link
Copy Markdown
Contributor

@ktrzcinx No, I can only figure out there is indeed some limitation when using flex structure in ELF files. The best solution is to avoid using this kind of structure in ELF sections.

@lgirdwood

lgirdwood commented Mar 30, 2020

Copy link
Copy Markdown
Member

Jenkins known issues.

@lgirdwood
lgirdwood merged commit d1b02d4 into thesofproject:masterMar 30, 2020
@mengdonglinmengdonglin added the ABI ABI change involved label Mar 31, 2020
@mengdonglinmengdonglin mentioned this pull request Mar 31, 2020
Closed
@lyakhlyakh mentioned this pull request Nov 27, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marc-hbmarc-hbmarc-hb left review comments
@dbalutadbalutadbaluta approved these changes
@paulstelian97paulstelian97paulstelian97 approved these changes
@lgirdwoodlgirdwoodlgirdwood approved these changes
@lbetlejlbetlejAwaiting requested review from lbetlej
@mmaka1mmaka1Awaiting requested review from mmaka1
@plbossartplbossartAwaiting requested review from plbossart
@tlaudatlaudaAwaiting requested review from tlauda
+1 more reviewer
@jajanuszjajanuszjajanusz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ABIABI change involved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ktrzcinx@paulstelian97@jajanusz@mmaka1@marc-hb@xiulipan@lgirdwood@dbaluta@mengdonglin