Regression(ed83797f3cbfc8): Behavior change in __weak in debug builds with -fno-objc-arc-exceptions

[nico]

Phab is down, so I can't look at the discussion at https://reviews.llvm.org/D105671. Maybe this is intentional!

We noticed that ed83797 changed the behavior of this code:

% cat test.mm
#import <Foundation/Foundation.h>

@class UILayoutGuide;

@interface LayoutGuideCenter : NSObject
- (UILayoutGuide * _Nonnull)makeLayoutGuideNamed:(NSString * _Nonnull)name;
@end

#define EXPECT_NE(a, b) if (a == b) return 1
#define EXPECT_EQ(a, b) if (a != b) return 2

int main() {
  LayoutGuideCenter* center_ = [[LayoutGuideCenter alloc] init];
  UILayoutGuide* layout_guide = [center_ makeLayoutGuideNamed:@"view"];

  EXPECT_NE(layout_guide, nil);

  // Create a weak reference to the layout guide, to check it gets nilled out.
  __weak UILayoutGuide* weak_layout_guide = layout_guide;

  @autoreleasepool {
    layout_guide = nil;
  }

  // This can fail if you are breaking in the debugger and inspecting the hash
  // table. This is due to NSHashTable not providing any guarantee as for when
  // the elements are released.
  EXPECT_EQ(weak_layout_guide, nil);
}

% cat center.mm
#import <Foundation/Foundation.h>

@interface UILayoutGuide : NSObject @end
@implementation UILayoutGuide @end

@interface LayoutGuideCenter : NSObject
- (UILayoutGuide * _Nonnull)makeLayoutGuideNamed:(NSString * _Nonnull)name;
@end

@implementation LayoutGuideCenter
- (UILayoutGuide * _Nonnull)makeLayoutGuideNamed:(NSString * _Nonnull)name
{
  return [[UILayoutGuide alloc] init];
}
@end

After the change, in non-optimized builds, this now exits with 2:

% third_party/llvm-build/Release+Asserts/bin/clang++  -isysroot $(xcrun -show-sdk-path) test.mm center.mm -fobjc-arc -fno-objc-arc-exceptions && ./a.out ; echo $?      
2

It exits with 0:

• when building with -O2
• when building without -fno-objc-arc-exceptions
• without ed83797

Is this an intentional change? It seems surprising that it only changes in some build configs.

The code looks a little bit weird, but possibly ok to me.

@jroelofs

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Nico Weber (nico)

Phab is down, so I can't look, so I can't look at the discussion at https://reviews.llvm.org/D105671. Maybe this is intentional!

We noticed that ed83797 changed the behavior of this code:

% cat test.mm
#import <Foundation/Foundation.h>

@<!-- -->class UILayoutGuide;

@<!-- -->interface LayoutGuideCenter : NSObject
- (UILayoutGuide * _Nonnull)makeLayoutGuideNamed:(NSString * _Nonnull)name;
@<!-- -->end

#define EXPECT_NE(a, b) if (a == b) return 1
#define EXPECT_EQ(a, b) if (a != b) return 2

int main() {
  LayoutGuideCenter* center_ = [[LayoutGuideCenter alloc] init];
  UILayoutGuide* layout_guide = [center_ makeLayoutGuideNamed:@"view"];

  EXPECT_NE(layout_guide, nil);

  // Create a weak reference to the layout guide, to check it gets nilled out.
  __weak UILayoutGuide* weak_layout_guide = layout_guide;

  @<!-- -->autoreleasepool {
    layout_guide = nil;
  }

  // This can fail if you are breaking in the debugger and inspecting the hash
  // table. This is due to NSHashTable not providing any guarantee as for when
  // the elements are released.
  EXPECT_EQ(weak_layout_guide, nil);
}

% cat center.mm
#import <Foundation/Foundation.h>

@<!-- -->interface UILayoutGuide : NSObject @<!-- -->end
@<!-- -->implementation UILayoutGuide @<!-- -->end

@<!-- -->interface LayoutGuideCenter : NSObject
- (UILayoutGuide * _Nonnull)makeLayoutGuideNamed:(NSString * _Nonnull)name;
@<!-- -->end

@<!-- -->implementation LayoutGuideCenter
- (UILayoutGuide * _Nonnull)makeLayoutGuideNamed:(NSString * _Nonnull)name
{
  return [[UILayoutGuide alloc] init];
}
@<!-- -->end

After the change, in non-optimized builds, this now exits with 2:

% third_party/llvm-build/Release+Asserts/bin/clang++  -isysroot $(xcrun -show-sdk-path) test.mm center.mm -fobjc-arc -fno-objc-arc-exceptions && ./a.out ; echo $?      
2

It exits with 0:

• when building with -O2
• when building without -fno-objc-arc-exceptions
• without ed83797

Is this an intentional change? It seems surprising that it only changes in some build configs.

The code looks a little bit weird, but possibly ok to me.

@jroelofs

[jroelofs]

I did not expect that change to affect program behavior. I'll have a look next week.

[nico]

Should we revert for now to keep trunk green?

[jroelofs]

Should we revert for now to keep trunk green?

Good idea. cb62f67

[jroelofs]

Looks like this is the relevant part of the diff responsible for the behavior change:

diff -u without-ed8379/test.s with-ed8379/test.s
--- without-ed8379/test.s       2023-10-20 09:54:41
+++ with-ed8379/test.s          2023-10-20 09:49:57
@@ -25,10 +25,12 @@
        adrp    x2, l__unnamed_cfstring_@PAGE
        add     x2, x2, l__unnamed_cfstring_@PAGEOFF
        bl      _objc_msgSend
+       str     x0, [sp, #32]                   ; 8-byte Folded Spill
        ; InlineAsm Start
        mov     x29, x29        ; marker for objc_retainAutoreleaseReturnValue
        ; InlineAsm End
        bl      _objc_retainAutoreleasedReturnValue
+       ldr     x0, [sp, #32]                   ; 8-byte Folded Reload
        stur    x0, [x29, #-24]
        ldur    x8, [x29, #-24]
        subs    x8, x8, #0