Skip to content

Revert "lib: newlib: Add workaround for #38258" #38733

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 24, 2021
Merged

Revert "lib: newlib: Add workaround for #38258" #38733

merged 2 commits into from
Sep 24, 2021

Conversation

stephanosio
Copy link
Member 

Revert "lib: newlib: Add workaround for #38258"

The commit 9bd1483afeb18f4225ec7b0340b0d4e20efb7d01 was added as a
workaround for the Xtensa initial malloc failure bug.

This bug has been fixed in the Zephyr SDK 0.13.1 release and therefore
this workaround is no longer needed.

Fixes #38258

NOTE: This PR also contains a commit to update the minimum required Zephyr SDK version to 0.13.1, which is required to support this.

@stephanosio
Revert "lib: newlib: Add workaround for zephyrproject-rtos#38258"
aa2600e 
The commit 9bd1483 was added as a
workaround for the Xtensa initial malloc failure bug.

This bug has been fixed in the Zephyr SDK 0.13.1 release and therefore
this workaround is no longer needed.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio
cmake: Set minimum required Zephyr SDK version to 0.13.1
fa81823 
This commit sets the minimum required Zephyr SDK version to 0.13.1,
which fixes the following critical issues:

1. Xtensa initial malloc failure (GitHub issue zephyrproject-rtos#38258)
2. ARMv8-M security extension vulnerability (CVE-2021-35465)

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio stephanosio marked this pull request as ready for review September 22, 2021 08:24
@stephanosio stephanosio requested a review from galak September 22, 2021 08:24
@stephanosio stephanosio added this to the v2.7.0 milestone Sep 22, 2021
@stephanosio stephanosio requested a review from cfriedt September 22, 2021 08:25
@stephanosio stephanosio added area: Toolchains Toolchains area: newlib Newlib C Standard Library TSC Topics that need TSC discussion labels Sep 22, 2021
tejlmand
tejlmand reviewed Sep 22, 2021
View reviewed changes
cmake/verify-toolchain.cmake
@@ -16,7 +16,7 @@
# FORMAT=json: Print the output as a json formatted string, useful for Python

# This is the minimum required Zephyr-SDK version which supports CMake package
set(TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION 0.13)
set(TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION 0.13.1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have earlier limited the requested Zephyr SDK based on arch when the SDK was a minor bump affecting only some architectures like here:

zephyr/cmake/verify-toolchain.cmake

Lines 19 to 23 in 835ff89

if("${ARCH}" STREQUAL "arm64")
set(TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION 0.12.4)
else()
set(TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION 0.12)
endif()

but as Zephyr 0.13.1 affects both xtensa and arm i'm also fine with just bumping for everyone.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so just placing this comment to get general opinions from everyone.

nashif
nashif previously requested changes Sep 22, 2021
View reviewed changes
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

this will break CI environments, please send an announcement and let people update their installed SDK first.

@galak
Copy link
Collaborator

galak commented Sep 22, 2021

this will break CI environments, please send an announcement and let people update their installed SDK first.

why do we need to worry about downstream CIs in this way?? where would you make such an annoucement

@galak
Copy link
Collaborator

galak commented Sep 22, 2021

@stephanosio we could split the commits in the PR so we could merge the revert while waiting on the SDK version bump.

@stephanosio
Copy link
Member Author

why do we need to worry about downstream CIs in this way?? where would you make such an annoucement

@galak I had a brief discussion with @nashif about this in #toolchain. We can further discuss this in the TSC meeting today.

@stephanosio
Copy link
Member Author

this will break CI environments, please send an announcement and let people update their installed SDK first.

@nashif I have sent out an announcement.

p.s. As discussed in the TSC meeting today, a formal process regarding these breaking changes will be discussed in the Process WG later.

@stephanosio stephanosio requested a review from nashif September 22, 2021 16:14
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 22, 2021 •
edited
Loading

why do we need to worry about downstream CIs in this way??

Should downstream never test Zephyr's main branch? Genuine question.

where would you make such an annoucement

In the usual Zephyr communication channels.

@nashif nashif removed the TSC Topics that need TSC discussion label Sep 22, 2021
@cfriedt cfriedt dismissed nashif’s stale review September 24, 2021 11:34

Announcement made

@cfriedt cfriedt merged commit 0ae9f5a into zephyrproject-rtos:main Sep 24, 2021
@marc-hb marc-hb mentioned this pull request Sep 27, 2021
Closed
marc-hb added a commit to marc-hb/sof that referenced this pull request Sep 27, 2021
@marc-hb
.github/zephyr: point at the latest zephyr container again
f8cc3d8 
Also stop hardcoding the SDK version number: Zephyr's CMake can
automatically find the latest SDK in the home directory but not in
/opt/toolchains so add some symbolic links.

Fixes the build broken since
zephyrproject-rtos/zephyr#38733

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this pull request Sep 28, 2021
@marc-hb
.github/zephyr: switch the container back to the :latest version
1dce542 
Tired of fixing Zephyr breakages all the time (latest in
zephyrproject-rtos/zephyr#38733)

Also unhardcode ZEPHYR_SDK_INSTALL_DIR thanks to the previous commit.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
lgirdwood pushed a commit to thesofproject/sof that referenced this pull request Sep 28, 2021
@marc-hb @lgirdwood
.github/zephyr: switch the container back to the :latest version
e104f84 
Tired of fixing Zephyr breakages all the time (latest in
zephyrproject-rtos/zephyr#38733)

Also unhardcode ZEPHYR_SDK_INSTALL_DIR thanks to the previous commit.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@cfriedt cfriedt cfriedt approved these changes

@enjiamai enjiamai enjiamai approved these changes

@tejlmand tejlmand tejlmand approved these changes

@galak galak Awaiting requested review from galak

@KangJianX KangJianX Awaiting requested review from KangJianX

@nashif nashif Awaiting requested review from nashif

Assignees

@tejlmand tejlmand

Labels
area: Build System area: C Library C Standard Library area: newlib Newlib C Standard Library area: Toolchains Toolchains
Projects
None yet
Milestone
v2.7.0
Development

Successfully merging this pull request may close these issues.

newlib: first malloc call may fail on Xtensa depending on image size
7 participants
@stephanosio @galak @marc-hb @nashif @cfriedt @enjiamai @tejlmand