Skip to content

Conversation

zeriyoshi
Copy link
Contributor

Clang 18 (stable) on AArch64 generates warnings about uninitialized values when using MSan.

This PR fixes the issue, but it's likely a false positive.
I'm not yet sure how to properly address the false positive, but I'm creating this draft PR for now.

@iluuu1994
Copy link
Member

Yeah, all of these are false positives. I don't think we should use memset. If there's no other way, it would be better to use __msan_unpoison_*.

@zeriyoshi
Copy link
Contributor Author

Indeed, replaced

@zeriyoshi zeriyoshi marked this pull request as ready for review September 12, 2024 13:19
@zeriyoshi zeriyoshi requested a review from bukka as a code owner September 12, 2024 13:19
@zeriyoshi
Copy link
Contributor Author

@iluuu1994
Please review this when you have some free time. No rush at all :)

result = zend_string_safe_alloc(((length + 2) / 3), 4 * sizeof(char), 0, 0);

# if __has_feature(memory_sanitizer)
/* Clang 18 MSan does not instrument on AArch64. */
Copy link
Member

@iluuu1994 iluuu1994 Sep 18, 2024

Choose a reason for hiding this comment

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

This comment is not accurate. Instrumentation does happen, just not correctly. I believe it's some intrinsic calls in neon_base64_encode/neon_base64_decode that actually lack instrumentation. So, you should likely move these unpoisons there. It might also be nice to move the __msan_unpoison to zend_portability.h with a conditional macro so that we can avoid the constant #include and __has_feature(memory_sanitizer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will try to move to zend_portability.h

# if __has_feature(memory_sanitizer)
/* Clang 18 MSan does not instrument on AArch64. */
__msan_unpoison(ZSTR_VAL(result), ZSTR_LEN(result));
# endif
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

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.

2 participants