Page MenuHomePhabricator

rsundahl (Roy Sundahl)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 30 2022, 12:35 PM (13 w, 1 d)

Recent Activity

Yesterday

rsundahl added a comment to D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..

ld64 starts doing this:

but should have read:

dyld starts doing this:

(Sorry for any confusion that this may have caused @MaskRay.) It is my understanding that there is no change in the static linker, only to the dynamic linker. I'm quite certain that we will not be reverting the change to the dyld as it is both required and has been shipping for the past year. Short of using XFAIL I have little choice than to set the MH_WEAK_DEFINES bit by including a weak symbol in the dylib. The weak symbol could well be the so-called "strong" symbol itself, but the optics of making a symbol weak in order to replace a different weak symbol may be somewhat counterintuitive when holding onto the "strong-replaces-weak" notions. That said, and as we discussed above, in the context of dynamic loading, it's not "strong-replaces-weak", it's "ignore-weak-attribute-and-use-first-encounter. (This is true with Darwin as it is with Linux and other dynamic loaders). With the clarification that this requirement is only for dyld on Darwin, would the conditional injection of a weak symbol on Apple platforms be acceptable? (Or alternatively, marking the intended override symbol as "weak"?) Thank you.

Thu, Jun 30, 8:06 PM · Restricted Project, Restricted Project

Mon, Jun 27

rsundahl committed rGd2dad6287cb3: Add wait for child processe(s) to exit. (amended+clang-formatted) (authored by rsundahl).
Add wait for child processe(s) to exit. (amended+clang-formatted)
Mon, Jun 27, 1:14 PM · Restricted Project, Restricted Project
rsundahl closed D128565: Add wait for child process(s) to exit..
Mon, Jun 27, 1:14 PM · Restricted Project, Restricted Project

Sat, Jun 25

rsundahl updated the diff for D128565: Add wait for child process(s) to exit..

Applied clang-format

Sat, Jun 25, 4:00 PM · Restricted Project, Restricted Project

Fri, Jun 24

rsundahl added a comment to D128565: Add wait for child process(s) to exit..

I think it's: #include <sys/wait.h>

Fri, Jun 24, 8:50 PM · Restricted Project, Restricted Project
rsundahl updated the diff for D128565: Add wait for child process(s) to exit..

Adding include of <sys/wait.h> for Linux.

Fri, Jun 24, 8:44 PM · Restricted Project, Restricted Project
rsundahl retitled D128565: Add wait for child process(s) to exit. from Add wait for child processe(s) to exit. to Add wait for child process(s) to exit..
Fri, Jun 24, 4:49 PM · Restricted Project, Restricted Project
rsundahl requested review of D128565: Add wait for child process(s) to exit..
Fri, Jun 24, 4:40 PM · Restricted Project, Restricted Project

Wed, Jun 22

rsundahl added a reviewer for D127929: [ubsan] Add weak symbol for weak-def coalescing consideration.: yln.
Wed, Jun 22, 2:52 PM · Restricted Project, Restricted Project

Thu, Jun 16

rsundahl added a comment to D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..

Thanks for confirmation. I think I understand the issue now.

Thank you for your consideration @MaskRay. I appreciate it. It's my understanding that the change is for efficiency and not security but could be both Even though this is an important discussion, at the end of the day, my concern is that even if all dynamic loaders behaved exactly the same, that behavior would not be defined by any C/C++ specifications that I know of and so would be undefined in those contexts. Therefore the original question still stands, "Should the sanitizers be dependent on the undefined behavior of weak symbols in a dynamic load environment? There are substantive differences in handling of dynamic weak symbols between openbsd, Windows, Linux and Apple.

Thu, Jun 16, 4:00 PM · Restricted Project, Restricted Project
rsundahl added a comment to D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..

Why will it fail with -mmacosx-version-min=12.0? Does ld64 start to drop MH_WEAK_DEFINES for non-weak-overriding-weak or does dyld start to do something different?

ld64 starts doing this:

Dyld uses that bit (if unset) to skip over scanning that dylib when looking for weak-defs

and so doesn't open the file and doesn't find the non-weak function so doesn't consider it. This commit adds an arbitrary weak symbol so the file will be included in the "weak-def-coalescing" process.

Thu, Jun 16, 2:42 PM · Restricted Project, Restricted Project
rsundahl added a comment to D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..

...and dyld works differently as described above. Consequently, the behavior is different between different dynamic loaders and what I originally referred to as "undefined". (A better word would have been un-specified.) In the absence of a specification for how weak symbols should be treated by dynamic loaders, I think the irony of UBSan itself depending on undefined behavior for it's own integrity checks, shouldn't be lost on anybody.

Thu, Jun 16, 1:09 PM · Restricted Project, Restricted Project
rsundahl added a comment to D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..

(@MaskRay, the test as is will fail when -mmacosx-version-min=12.0 and up)

Thu, Jun 16, 12:15 PM · Restricted Project, Restricted Project

Wed, Jun 15

rsundahl added a comment to D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..

I included all of you because while this is a small change for an Apple issue, it stems from the fundamental reliance on the behavior of the linker and how it resolves multiple symbols in the context of dynamic linking. This behavior is not spelled out by any language specification and I wanted to stimulate some thinking on whether or not we should shed the dependence on weak definitions completely and move to more robust and explicit say callback registration. In this simple file, you can see that Apple, wine and openbsd all have their issues with weak symbols and are choosing to opt out.

Wed, Jun 15, 7:21 PM · Restricted Project, Restricted Project
rsundahl requested review of D127929: [ubsan] Add weak symbol for weak-def coalescing consideration..
Wed, Jun 15, 7:15 PM · Restricted Project, Restricted Project

May 23 2022

rsundahl accepted D126229: [Sanitizer][Darwin] Add explanation for Apple platform macros.
May 23 2022, 11:55 AM · Restricted Project, Restricted Project
rsundahl added a comment to D126229: [Sanitizer][Darwin] Add explanation for Apple platform macros.

LGTM (I'd like to see SANTIZER_OSX become SANITIZER_MACOS at some point for consistency.)

May 23 2022, 11:55 AM · Restricted Project, Restricted Project

May 18 2022

rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

Fix (and cleanup) for failure of CodeGen arm.c check in ci/cd pipeline.

May 18 2022, 11:24 AM · Restricted Project, Restricted Project, Restricted Project

May 13 2022

rsundahl added inline comments to D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..
May 13 2022, 2:24 PM · Restricted Project, Restricted Project, Restricted Project
rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

This update corrects merge conflicts in Build #104091

May 13 2022, 2:12 PM · Restricted Project, Restricted Project, Restricted Project

May 12 2022

rsundahl added a comment to D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

The update completes the suggested changes. The generated code is slightly different around initialization of the array cookie due to choosing one implementation over another when I "folded" ARMCXXABI::InitializeArrayCookie() into ItaniumCXXABI::InitializeArrayCookie(). The generated code LGTM but I would appreciate added scrutiny for the ItaniumCXXABI.cpp changes. check-sanitizer and check-asan completed successfully on x86_64, arm64 and arm64e.

May 12 2022, 9:23 PM · Restricted Project, Restricted Project, Restricted Project
rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..
  • Refactor InitializeArrayCookie and readArrayCookieImpl.
May 12 2022, 8:57 PM · Restricted Project, Restricted Project, Restricted Project

May 11 2022

rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

Update diff back to dc7e02d4b4dc30d44ddebd832719a6e63396e718

May 11 2022, 11:40 AM · Restricted Project, Restricted Project, Restricted Project
rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

Revert ItaniumCXXABI.cpp for now (unintentional push)

May 11 2022, 11:19 AM · Restricted Project, Restricted Project, Restricted Project
rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

Implement suggestions from reviews. (Incremental update.)

May 11 2022, 11:00 AM · Restricted Project, Restricted Project, Restricted Project
rsundahl added inline comments to D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..
May 11 2022, 7:18 AM · Restricted Project, Restricted Project, Restricted Project

May 9 2022

rsundahl added a comment to D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

Adding dialog to comments made by reviewers.

May 9 2022, 1:25 PM · Restricted Project, Restricted Project, Restricted Project
rsundahl added reviewers for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.: vitalybuka, kcc.

Adding Vitaly Buka and Kostya Serebryany (sanitizer maintainers).

May 9 2022, 10:41 AM · Restricted Project, Restricted Project, Restricted Project
rsundahl updated the diff for D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..

Adding ASAN test "new_array_cookie_with_new_from_class" (rdar://92884511)

May 9 2022, 8:42 AM · Restricted Project, Restricted Project, Restricted Project

May 8 2022

rsundahl requested review of D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks..
May 8 2022, 5:09 PM · Restricted Project, Restricted Project, Restricted Project

May 3 2022

rsundahl accepted D124591: tsan: for unittests, change to use test fixtures to clear racy stacks.

Thanks for this update @ychen , it fixes our downstream tsan>RaceWithDeadThread failure since the original commit and gets to the root cause nicely..

May 3 2022, 9:38 AM · Restricted Project, Restricted Project

Apr 21 2022

rsundahl committed rGd179627ef012: Fix sanitizer stack traces on aarch64. (authored by rsundahl).
Fix sanitizer stack traces on aarch64.
Apr 21 2022, 2:28 PM · Restricted Project, Restricted Project
rsundahl closed D124140: Fix sanitizer stack traces on aarch64..
Apr 21 2022, 2:28 PM · Restricted Project, Restricted Project

Apr 20 2022

rsundahl updated the diff for D124140: Fix sanitizer stack traces on aarch64..

Fixed markup issue (double underlines) in summary.

Apr 20 2022, 6:58 PM · Restricted Project, Restricted Project
rsundahl requested review of D124140: Fix sanitizer stack traces on aarch64..
Apr 20 2022, 6:41 PM · Restricted Project, Restricted Project
rsundahl added a comment to D123736: [Darwin][ASan][Sanitizer] Fixes Sanitizer NonUnique Identifier to Account for Mac arm64 architectures..

It may be more canonical to use:

#if SANITIZER_MAC && SANITIZER_X64

Reasoning: While _M_X64 is included in SANITIZER_X64 (along with x86_64), it's Windows-only and is clearly intended as an alias or alternate for x86_64 w.r.t. sanitizer.

Apr 20 2022, 8:39 AM · Restricted Project, Restricted Project

Apr 5 2022

rsundahl committed rG47e7a2247115: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets. (authored by rsundahl).
[Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets.
Apr 5 2022, 12:22 PM · Restricted Project, Restricted Project
rsundahl closed D123099: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets..
Apr 5 2022, 12:21 PM · Restricted Project, Restricted Project
rsundahl accepted D123099: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets..

Self-approved with comment.

Apr 5 2022, 5:00 AM · Restricted Project, Restricted Project
rsundahl added inline comments to D123099: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets..
Apr 5 2022, 4:49 AM · Restricted Project, Restricted Project
rsundahl updated the diff for D123099: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets..
  1. Updated D123099: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets.
  2. Fixed nits
Apr 5 2022, 4:45 AM · Restricted Project, Restricted Project

Apr 4 2022

rsundahl requested review of D123099: [Darwin][ASan][Sanitizer] Enable dlclose-test for all darwin targets..
Apr 4 2022, 4:34 PM · Restricted Project, Restricted Project