Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Revert "lib c/cpp: Move .ctor .init_array handling from C++ to kernel" #75399

Closed
wants to merge 1 commit into from

Conversation

andyross
Copy link
Contributor

@andyross andyross commented Jul 3, 2024

Unfortunately this breaks native_sim fuzzing due (presumably) to interactions with the .ctors emitted by the libc/sanitizer layer.

See SOF discussion at thesofproject/sof#9278

This reverts commit 6e977ae.

Unfortunately this breaks native_sim fuzzing due (presumably) to
interactions with the .ctors emitted by the libc/sanitizer layer.

See SOF discussion at thesofproject/sof#9278

This reverts commit 6e977ae.

Signed-off-by: Andy Ross <andyross@google.com>
@andyross
Copy link
Contributor Author

andyross commented Jul 3, 2024

cc @kv2019i @aescolar @marc-hb

Throwing this up as a revert since we're so close to the release just to have somewhere to discuss the regression. I don't have a proper analysis yet.

@aescolar
Copy link
Member

aescolar commented Jul 3, 2024

@andyross you are still running the fuzzer in native_posix and not native_sim?
(i can repro the issue in native_posix but things seem to work fine in native_sim)

EDIT: you are :) (just checked your CI)

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

I see the reason for the issue1
It would seem this only happens when you run the fuzzer in native_posix but works fine in native_sim.
So I see as less dramatic options:
a) We just say that the fuzzer is not supported anymore in native_posix but only in native_sim. (native_posix will be deprecated right after we do the release). So we are just forcing you to switch the target in your CI by 3 weeks :)
b) We change the default so this option is not enabled for NATIVE_APPLICATION if ARCH_POSIX_LIBFUZZER

Footnotes

  1. native_posix does not have a separately linked runner where its own constructors will be run like native_sim, so indeed whatever the fuzzer wanted to run before the native_posix initialization is not run yet and it fails

@andyross
Copy link
Contributor Author

andyross commented Jul 3, 2024

OK, will investigate what needs to change to get the fuzzing support ported to native_sim. For right now it's probably enough to add a kconfig setting to SOF to turn this off (which would preclude fuzzing C++ code or relying on non-C++ static initializers of course, but that's fine for the short term).

FWIW the deadline is actually now, though. SOF tracks upstream pretty closely and the linked discussion is an uprev required to get an unrelated fix.

@aescolar
Copy link
Member

aescolar commented Jul 3, 2024

ARCH_POSIX_LIBFUZZER

fastest fix just do CONFIG_STATIC_INIT_GNU=n in your prj.conf
EDIT: and
CONFIG_TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU=n
and add CONFIG_TOOLCHAIN_SUPPORTS_STATIC_INIT_GNU to the Kconfig of the app with a prompt so it is user selectable.. :)

@andyross
Copy link
Contributor Author

andyross commented Jul 3, 2024

Yeah, I was bumping into the kconfig trap too. :) Really the fastest way to solve this (absent SOF carrying around a private reversion) is to just do the board swap.

@aescolar
Copy link
Member

aescolar commented Jul 3, 2024

@andyross #75404

@keith-packard
Copy link
Collaborator

Looks like there are a couple of fine options to pick from; not sure I can help you all decide which to run with.

@andyross
Copy link
Contributor Author

andyross commented Jul 3, 2024

Going to close this one. native_sim port in SOF is up at thesofproject/sof#9280 which resolves the regression for me, no need to do a full revert before LTS

@andyross andyross closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants