Adds entry points to the runtime that exposes metadata #32339

augusto2112 · 2020-06-12T01:09:25Z

This PR will add the necessary functionality in order to enable SwiftReflectionTest to work on Linux. This will be done by adding entry points in the Swift runtime that only build in debug builds

vedantk · 2020-06-12T21:07:20Z

@augusto2112 per our discussion on the forums, do we need to extend the swift_reflection_addReflectionInfo interface to handle non-contiguous section layouts?

augusto2112 · 2020-06-12T21:26:25Z

@vedantk I think we have two possibilities:

Extend the swift_reflection_addReflectionInfo interface to handle non-contiguous section layouts
Implement “getAddressInfoForImage”, which returns the address and start of the __TEXT segment in Mach-O (so .text in Elf?). I’d have to write a new runtime entry point that returns this information (which is where I was stuck before). This way, we can ditch swift_reflection_info and swift_reflection_addReflectionInfo, which are deprecated, in favor of swift_reflection_addImage.

What do you think? Maybe we should consult Joe and Slava on which way to go?

vedantk · 2020-06-13T00:37:00Z

To summarize our video chat conversation:

I think adding an API like swift_reflection_addNonContiguousReflectionInfo is the way to go. Past experience suggests that implementing getAddressInfoForImage could be fragile, and we can't change swift_reflection_addReflectionInfo itself because it's fixed API.
It looks like https://github.com/apple/swift/blob/master/docs/ABI/TypeMetadata.rst#class-metadata needs to be updated to describe the non-ObjC Interop ABI, which it turns out is not the same as the ObjC Interop ABI ;-)

vedantk · 2020-06-13T00:37:56Z

Also: cc'ing @jckarter @slavapestov for visibility.

augusto2112 · 2020-06-14T17:28:03Z

@vedantk I've updated the PR to support the non-contiguous reflection info 🎉

Now a new problem has shown up: inside TypeRefBuilder::getFieldTypeInfo, we look for the Field Descriptor that references TestClass defined inside reflect_UInt16.swift, with mangled name "14reflect_UInt169TestClassC", by iterating over the FieldSection of all our ReflectionInfos. The problem is, this particular Field Descriptor is not there. I took a look at the Field Descriptors we do find, and the Field Descriptors of the types defined in SwiftReflectionTest (InstanceKind, Section, etc) are present, but not the one defined inside reflect_UInt16. As a test, I experimented with moving the definition of TestClass to SwiftReflectionTest, and then we can actually look it up. So we need to find out why the definitions declared outside SwiftReflectionTest are not present in any Field Sections. (fixed, it was a bug in my code, where I wasn't sending the last Image)

Edit: So now, we actually get to print the type info! Although the output is a bit different from what is expected in the test.
Actual output:

Type info:
(class_instance size=20 alignment=2 stride=20 num_extra_inhabitants=0 bitwise_takable=1
  (field name=t offset=18
    (struct size=2 alignment=2 stride=2 num_extra_inhabitants=0 bitwise_takable=1
      (field name=_value offset=0
        (builtin size=2 alignment=2 stride=2 num_extra_inhabitants=0 bitwise_takable=1)))))

Expected output:

 Type info:
 (class_instance size=18 alignment=2 stride=18 num_extra_inhabitants=0 bitwise_takable=1
   (field name=t offset=16
     (struct size=2 alignment=2 stride=2 num_extra_inhabitants=0 bitwise_takable=1
       (field name=_value offset=0
         (builtin size=2 alignment=2 stride=2 num_extra_inhabitants=0 bitwise_takable=1)))))

So the class_instance size is 18 instead of 20. ~~This value matches the instance size field in the class metadata~~ (I got confused, the metadata size is actually 18) , which leads me to believe it is correct, and the difference might be due to the lack of Objective-C interop, or some other platform difference. Are there any docs that describe how the class_instance size is calculated? It feels like we're very very close now!

include/swift/Remote/MetadataReader.h

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

adrian-prantl · 2020-06-15T17:24:36Z

@jckarter Does it look plausible to you that the class would be 2 bytes larger on Linux, or do you suspect a bug?

adrian-prantl · 2020-06-15T17:26:35Z

@augusto2112 Can you dump the LLVM IR for the Darwin and the Linux version of the class? I'm not sure if this is static enough to show the difference, but if it does allow us to see the fields, we could probably figure out where the difference comes from and whether it's plausible.

augusto2112 · 2020-06-15T17:46:43Z

@adrian-prantl Here's the relevant IR for the class on Linux:

@"$s4test9TestClassC1ts6UInt16VvpWvd" = hidden constant i64 16, align 8
@"$sBoWV" = external global i8*, align 8
@0 = private constant [5 x i8] c"test\00"
@"$s4testMXM" = linkonce_odr hidden constant <{ i32, i32, i32 }> <{ i32 0, i32 0, i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @0 to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32 }>, <{ i32, i32, i32 }>* @"$s4testMXM", i32 0, i32 2) to i64)) to i32) }>, section ".rodata", align 4
@1 = private constant [10 x i8] c"TestClass\00"
@"$s4test9TestClassCMn" = hidden constant <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }> <{ i32 -2147483568, i32 trunc (i64 sub (i64 ptrtoint (<{ i32, i32, i32 }>* @"$s4testMXM" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 1) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([10 x i8]* @1 to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 2) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (%swift.metadata_response (i64)* @"$s4test9TestClassCMa" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 3) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ({ i32, i32, i16, i16, i32, i32, i32, i32 }* @"$s4test9TestClassCMF" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 4) to i64)) to i32), i32 0, i32 2, i32 12, i32 5, i32 1, i32 7, i32 8, i32 4, %swift.method_descriptor { i32 18, i32 trunc (i64 sub (i64 ptrtoint (i16 (%T4test9TestClassC*)* @"$s4test9TestClassC1ts6UInt16Vvg" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 13, i32 1) to i64)) to i32) }, %swift.method_descriptor { i32 19, i32 trunc (i64 sub (i64 ptrtoint (void (i16, %T4test9TestClassC*)* @"$s4test9TestClassC1ts6UInt16Vvs" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 14, i32 1) to i64)) to i32) }, %swift.method_descriptor { i32 20, i32 trunc (i64 sub (i64 ptrtoint ({ i8*, %Ts6UInt16V* } (i8*, %T4test9TestClassC*)* @"$s4test9TestClassC1ts6UInt16VvM" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 15, i32 1) to i64)) to i32) }, %swift.method_descriptor { i32 1, i32 trunc (i64 sub (i64 ptrtoint (%T4test9TestClassC* (i16, %swift.type*)* @"$s4test9TestClassC1tACs6UInt16V_tcfC" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i32 0, i32 16, i32 1) to i64)) to i32) } }>, section ".rodata", align 4
@"$s4test9TestClassCMf" = internal global <{ void (%T4test9TestClassC*)*, i8**, i64, %swift.type*, i32, i32, i32, i16, i16, i32, i32, 
<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>*, i8*, i64, i16 (%T4test9TestClassC*)*, void (i16, %T4test9TestClassC*)*, { i8*, %Ts6UInt16V* } (i8*, %T4test9TestClassC*)*, %T4test9TestClassC* (i16, %swift.type*)* }> 
<{ void (%T4test9TestClassC*)* @"$s4test9TestClassCfD", i8** @"$sBoWV", i64 0, %swift.type* null, i32 2, i32 0, i32 18, i16 7, i16 0, i32 112, i32 16, <{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn", i8* null, i64 16, i16 (%T4test9TestClassC*)* @"$s4test9TestClassC1ts6UInt16Vvg", void (i16, %T4test9TestClassC*)* @"$s4test9TestClassC1ts6UInt16Vvs", { i8*, %Ts6UInt16V* } (i8*, %T4test9TestClassC*)* @"$s4test9TestClassC1ts6UInt16VvM", %T4test9TestClassC* (i16, %swift.type*)* @"$s4test9TestClassC1tACs6UInt16V_tcfC" }>, align 8
@"symbolic _____ 4test9TestClassC" = linkonce_odr hidden constant <{ i8, i32, i8 }> <{ i8 1, i32 trunc (i64 sub (i64 ptrtoint (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i8, i32, i8 }>, <{ i8, i32, i8 }>* @"symbolic _____ 4test9TestClassC", i32 0, i32 1) to i64)) to i32), i8 0 }>, section "swift5_typeref", align 2
@"$ss6UInt16VMn" = external global %swift.type_descriptor, align 4
@"got.$ss6UInt16VMn" = private unnamed_addr constant %swift.type_descriptor* @"$ss6UInt16VMn"
@"symbolic _____ s6UInt16V" = linkonce_odr hidden constant <{ i8, i32, i8 }> <{ i8 2, i32 trunc (i64 sub (i64 ptrtoint (%swift.type_descriptor** @"got.$ss6UInt16VMn" to i64), i64 ptrtoint (i32* getelementptr inbounds (<{ i8, i32, i8 }>, <{ i8, i32, i8 }>* @"symbolic _____ s6UInt16V", i32 0, i32 1) to i64)) to i32), i8 0 }>, section "swift5_typeref", align 2
@2 = private constant [2 x i8] c"t\00", section "swift5_reflstr"
@"$s4test9TestClassCMF" = internal constant { i32, i32, i16, i16, i32, i32, i32, i32 } { i32 trunc (i64 sub (i64 ptrtoint (<{ i8, i32, i8 }>* @"symbolic _____ 4test9TestClassC" to i64), i64 ptrtoint ({ i32, i32, i16, i16, i32, i32, i32, i32 }* @"$s4test9TestClassCMF" to i64)) to i32), i32 0, i16 1, i16 12, i32 1, i32 2, i32 trunc (i64 sub (i64 ptrtoint (<{ i8, i32, i8 }>* @"symbolic _____ s6UInt16V" to i64), i64 ptrtoint (i32* getelementptr inbounds ({ i32, i32, i16, i16, i32, i32, i32, i32 }, { i32, i32, i16, i16, i32, i32, i32, i32 }* @"$s4test9TestClassCMF", i32 0, i32 6) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([2 x i8]* @2 to i64), i64 ptrtoint (i32* getelementptr inbounds ({ i32, i32, i16, i16, i32, i32, i32, i32 }, { i32, i32, i16, i16, i32, i32, i32, i32 }* @"$s4test9TestClassCMF", i32 0, i32 7) to i64)) to i32) }, section "swift5_fieldmd", align 4
@"\01l_type_metadata_table" = private constant [1 x %swift.type_metadata_record] [%swift.type_metadata_record 
{ i32 trunc (i64 sub (i64 ptrtoint (<{ i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor, %swift.method_descriptor }>* @"$s4test9TestClassCMn" to i64), i64 ptrtoint ([1 x %swift.type_metadata_record]* 
@"\01l_type_metadata_table" to i64)) to i32) }], section "swift5_type_metadata", align 4
@__swift_reflection_version = linkonce_odr hidden constant i16 3
@_swift1_autolink_entries = private constant [63 x i8] c"-lswiftSwiftOnoneSupport\00-lswiftCore\00-lpthread\00-lutil\00-ldl\00-lm\00", section ".swift1_autolink_entries", align 8
@llvm.used = appending global [4 x i8*] [i8* bitcast ({ i32, i32, i16, i16, i32, i32, i32, i32 }* @"$s4test9TestClassCMF" to i8*), i8* bitcast ([1 x %swift.type_metadata_record]* @"\01l_type_metadata_table" to i8*), i8* bitcast (i16* @__swift_reflection_version to i8*), i8* getelementptr inbounds ([63 x i8], [63 x i8]* @_swift1_autolink_entries, i32 0, i32 0)], section "llvm.metadata", align 8

I believe the the instance size field in the metadata is 18.
Here's where I got the size from:

<{ void (%T4test9TestClassC*)* @"$s4test9TestClassCfD", i8** @"$sBoWV", // destructor
i64 0,  // isa
%swift.type* null, // super
i32 2, // class flags
i32 0, // instance addres pointer
i32 18, // instance size
...

I'm having trouble compiling the class for darwin, it seems that I need the standard library for the OS, which I don't have... I'd be grateful if someone could compile it for me so we can check compare them

augusto2112 · 2020-06-15T18:35:57Z

@vedantk, @adrian-prantl I'm thinking that I'm returning the wrong thing in readInstanceStartAndAlignmentFromClassMetadata. I'm returning the instance size, but maybe this should be the start of the object after the isa and super pointers (8 + 8) = 16. This produces the correct output (at least for this test) if I change the start to be 16. What do you think?

adrian-prantl · 2020-06-15T19:36:38Z

Be sure to try this with classes of different sizes.

augusto2112 · 2020-06-15T19:46:09Z

@adrian-prantl, @vedantk

I think this is it. I adapted the algorithm to work with inherits_Swift and it's passing.

This is the current status of the tests:

Failing Tests (21):
    Swift(linux-x86_64) :: Reflection/existentials.swift
    Swift(linux-x86_64) :: Reflection/existentials_objc.swift
    Swift(linux-x86_64) :: Reflection/functions.swift
    Swift(linux-x86_64) :: Reflection/functions_objc.swift
    Swift(linux-x86_64) :: Reflection/inherits_NSObject.swift
    Swift(linux-x86_64) :: Reflection/inherits_ObjCClasses.swift
    Swift(linux-x86_64) :: Reflection/reflect_Array.swift
    Swift(linux-x86_64) :: Reflection/reflect_Character.swift
    Swift(linux-x86_64) :: Reflection/reflect_Dictionary.swift
    Swift(linux-x86_64) :: Reflection/reflect_Enum_SingleCasePointerPayload.swift
    Swift(linux-x86_64) :: Reflection/reflect_Enum_TwoCaseOnePointerPayload.swift
    Swift(linux-x86_64) :: Reflection/reflect_Enum_value.swift
    Swift(linux-x86_64) :: Reflection/reflect_Enum_values.swift
    Swift(linux-x86_64) :: Reflection/reflect_NSArray.swift
    Swift(linux-x86_64) :: Reflection/reflect_NSNumber.swift
    Swift(linux-x86_64) :: Reflection/reflect_NSSet.swift
    Swift(linux-x86_64) :: Reflection/reflect_NSString.swift
    Swift(linux-x86_64) :: Reflection/reflect_Set.swift
    Swift(linux-x86_64) :: Reflection/reflect_String.swift
    Swift(linux-x86_64) :: Reflection/reflect_existential.swift
    Swift(linux-x86_64) :: Reflection/reflect_multiple_types.swift

  Expected Passes    : 24
  Unsupported Tests  : 2
  Unexpected Failures: 21

I think we can divide them in 3 camps:

ObjC ones (existentials_objc, inherits_ObjCClasses, etc), which I believe are expected to fail.
Foundation ones (reflect_NSSet, reflect_NSNumber, etc), which, since I don't have Foundation compiled right now, should also fail (when I started this issue, Foundation was broken on Linux. I'll fast-forward my branch and try compiling Foundation tonight, to test those out).
Others (existentials, reflect_Array, etc). These should be passing but are failing, so I'll take a look at them now.

adrian-prantl · 2020-06-15T20:11:53Z

You should be able to mark up the Objective-C ones with // REQUIRES: objc_interop. The Foundation ones will be interesting, since the Linux Foundation is a Swift reimplementation of (NS)Foundation so the layouts may actually be legitimately different. In these cases you'll need to mark them as // REQUIRES: objc_interop, too.

include/swift/Remote/MetadataReader.h

stdlib/tools/swift-reflection-test/swift-reflection-test.c

stdlib/public/SwiftShims/RuntimeShims.h

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

vedantk · 2020-06-15T21:49:17Z

Are there any docs that describe how the class_instance size is calculated? It feels like we're very very close now!

Really nice work! Yes, we're quite close now. I suggest working backwards from where we dump class_instance size=20 in the runtime (this should be '18' right?).

augusto2112 · 2020-06-15T21:57:14Z

Really nice work! Yes, we're quite close now. I suggest working backwards from where we dump class_instance size=20 in the runtime (this should be '18' right?).

@vedantk I believe I figured this one out. I just pushed a change that gives us the correct output. Basically, I think that readInstanceStartAndAlignmentFromClassMetadata should return the start of our fields in our class. When a class has no super classes, this offset is after two pointers worth of data (1 for the isa and 1 for retain counts). If it does have super-classes, we add the size of the super-classes' fields. This matches up with what is expected in the inherits_Swift test (and indeed that test is passing now).

vedantk

Awesome, then I think it's time to push this PR out of the work-in-progress stage :).

include/swift/Remote/MetadataReader.h

include/swift/Remote/RemoteAddress.h

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

stdlib/public/runtime/ImageInspectionELF.cpp

augusto2112 · 2020-06-15T22:30:14Z

Awesome, then I think it's time to push this PR out of the work-in-progress stage :).

Should I push enabling all the test cases? It won't work yet, but it would nice to see what's still failing on the CI.

vedantk · 2020-06-15T23:57:29Z

Should I push enabling all the test cases? It won't work yet, but it would nice to see what's still failing on the CI.

Go for it 👍

compnerd · 2020-07-15T15:46:09Z

include/swift/SwiftRemoteMirror/SwiftRemoteMirror.h

@@ -80,6 +80,12 @@ void
 swift_reflection_addReflectionInfo(SwiftReflectionContextRef ContextRef,
                                   swift_reflection_info_t Info);

+/// Add reflection sections that may not be contiguous from a loaded Swift image.
+SWIFT_REMOTE_MIRROR_LINKAGE
+void swift_reflection_addNonContiguousReflectionInfo(


Is there a conflicting interface for swift_reflection_addReflectionInfo? I'm not sure I see the value in the NonContiguous here. Or was there some other reason that you opted to add that?

The difference is that this receives a swift_non_contiguous_reflection_info_t instead of a swift_reflection_info_t. I needed to change the data structure because swift_reflection_info_t expects the offset of the local and remote sections to be the same (because it assumed the sections were contiguous, which isn't true).

swift_reflection_addReflectionInfo already exists, and @vedantk asked me to not change it since it's part of the API.

I believe swift_reflection_info_t isn't sufficiently general to describe a non-contiguous section layout.

Under what circumstances are you seeing a non-contiguous metadata section?

@jckarter in my tests on Linux, they always appear to be non-contiguous. Maybe it's OS related?

Ah, I see what you mean, the subsections are contiguous, but the mapping from remote to local addresses between them may not be 1:1. That's reasonable. As the comments note, swift_reflection_addReflectionInfo is deprecated and only exists for legacy ABI reasons on Apple platforms, because it's unable to accurately express this reality. We could probably remove it entirely on non-Apple platforms. It would be great if we could name this new API in a way that makes it more apparent it should be the preferred API for new code using Remote Mirror that wants to manually register images.

I might be wrong, but from my tests I believe the subsections really weren't contiguous. I'll re-check this to see if I'm correct.

@jckarter here is what swift-reflection-test.c logs in my machine (I just added the End field, so it's easier to visualize):

makeRemoteSection: Making remote section with Start = 0x7f22a7aee7b0 End = 0x7f22a7af285c and Size = 16556 makeRemoteSection: Making remote section with Start = 0x7f22a7ae99a8 End = 0x7f22a7aee7b0 and Size = 19976 makeRemoteSection: Making remote section with Start = 0x7f22a7a13f1c End = 0x7f22a7a14a98 and Size = 2940 makeRemoteSection: Making remote section with Start = 0x7f22a7ae8f40 End = 0x7f22a7ae99a8 and Size = 2664 makeRemoteSection: Making remote section with Start = 0x7f22a7a0d864 End = 0x7f22a7a12e43 and Size = 21983 makeRemoteSection: Making remote section with Start = 0x7f22a7a12e50 End = 0x7f22a7a13f19 and Size = 4297 makeRemoteSection: Making remote section with Start = 0x7f22a7334244 End = 0x7f22a7334244 and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a7334244 End = 0x7f22a7334244 and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a7334244 End = 0x7f22a7334244 and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a7334244 End = 0x7f22a7334244 and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a7349180 End = 0x7f22a734927c and Size = 252 makeRemoteSection: Making remote section with Start = 0x7f22a7334244 End = 0x7f22a7334244 and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a74f640c End = 0x7f22a74f6a40 and Size = 1588 makeRemoteSection: Making remote section with Start = 0x7f22a74f63f4 End = 0x7f22a74f640c and Size = 24 makeRemoteSection: Making remote section with Start = 0x7f22a74f3b1c End = 0x7f22a74f3b1c and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a74f3b1c End = 0x7f22a74f3b1c and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a74f63d0 End = 0x7f22a74f63f3 and Size = 35 makeRemoteSection: Making remote section with Start = 0x7f22a74f36a9 End = 0x7f22a74f3b05 and Size = 1116 makeRemoteSection: Making remote section with Start = 0x7f22a7b25344 End = 0x7f22a7b254a4 and Size = 352 makeRemoteSection: Making remote section with Start = 0x7f22a7b2530c End = 0x7f22a7b25344 and Size = 56 makeRemoteSection: Making remote section with Start = 0x7f22a7b21cce End = 0x7f22a7b21cce and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a7b21cce End = 0x7f22a7b21cce and Size = 0 makeRemoteSection: Making remote section with Start = 0x7f22a7b25228 End = 0x7f22a7b25309 and Size = 225 makeRemoteSection: Making remote section with Start = 0x7f22a7b21c00 End = 0x7f22a7b21cbe and Size = 190 makeRemoteSection: Making remote section with Start = 0x560e295150dc End = 0x560e295150f8 and Size = 28 makeRemoteSection: Making remote section with Start = 0x560e295131ee End = 0x560e295131ee and Size = 0 makeRemoteSection: Making remote section with Start = 0x560e295131ee End = 0x560e295131ee and Size = 0 makeRemoteSection: Making remote section with Start = 0x560e295131ee End = 0x560e295131ee and Size = 0 makeRemoteSection: Making remote section with Start = 0x560e295150d0 End = 0x560e295150dc and Size = 12 makeRemoteSection: Making remote section with Start = 0x560e295131ec End = 0x560e295131ee and Size = 2

I think the second block is the easiest to look at, since all sections, except 1, have size 0. The old algorithm would mistakenly assume that the sections would go from address 0x7f22a7334244 up to 0x7f22a734927c, which is 15038 bytes long.

Sorry, by "subsection" I mean one of the individual areas referenced in the reflection_info struct, like field, associated_types, and so on. Anyway, the swift_reflection_info and swift_reflection_addReflectionInfo interfaces are obsolete, and I'm saying it would be good to name these new ones swift_reflection_addReflectionInfo2 or something like that to make it clear it should supersede the old interface.

compnerd · 2020-07-15T15:47:06Z

include/swift/SwiftRemoteMirror/SwiftRemoteMirrorTypes.h

@@ -48,11 +48,22 @@ typedef struct swift_reflection_section {
  void *End;
 } swift_reflection_section_t;

+/// Represents the remote address and size of an image.
+typedef struct swift_remote_reflection_section {


Is this actually a section as per the file format or is this really just the specific metadata table?

This is supposed to represent the counterpart of the already existing swift_reflection_section struct. That's why I named it swift_remote_reflection_section. Is this name wrong? Aren't associated_types, builtin_types, capture, etc sections?

They are sections only on ELF because of the restrictions from the file format. Each one of those is a separate metadata table. In fact, if you look at MachO, they are all in the __DATA section and each is a segment. PE/COFF does something similar - entries are grouped, and then emitted entirely into the .data section. If this is meant to be ELF specific, please put that into the name (e.g. swift_remote_reflection_elf_section).

You have that backwards, __DATA is the segment (they're actually in __TEXT, though, because they're constant), and __TEXT, __swift5_foo is the section, in Mach-O parlance. The name seems correct to me.

Ah, okay, if that is constant across all the formats, sounds reasonable.

compnerd · 2020-07-15T15:47:57Z

include/swift/SwiftRemoteMirror/SwiftRemoteMirrorTypes.h

@@ -48,11 +48,22 @@ typedef struct swift_reflection_section {
  void *End;
 } swift_reflection_section_t;

+/// Represents the remote address and size of an image.


I don't think that this really describes the structure and its usage. The address and size would be inferred from the structure itself. What does it actually represent? I believe that it is really a metadata table chunk.

You're right. Maybe "/// Represents the remote address and size of an image's section."?

Like the previous comment, I don't think that the term section should play into this. I would say that it represents a (contiguous) region in the image's address space.

compnerd · 2020-07-15T15:49:04Z

include/swift/SwiftRemoteMirror/SwiftRemoteMirrorTypes.h

 typedef struct swift_reflection_section_pair {
  swift_reflection_section_t section;
  swift_reflection_ptr_t offset; ///< DEPRECATED. Must be zero
 } swift_reflection_section_pair_t;

+typedef struct swift_local_and_remote_reflection_section {


Again, Im not sure what section here really refers to. Given the type, I believe that this is really the bidirectional mapping from local to remote? Why not something like swift_remote_reflection_metadata_mapping?

I used section as this was the existing nomenclature I found. This matches with what's in SwiftReflectionTest (a ReflectionInfo contains many Sections) and the exisiting swift_reflection_section as well. Is the name "section" wrong here? If not, what about swift_reflection_section_mapping, would that be better?

Yeah, I don't think that it really is correct to encode the details of a specific file format into the name. The nomenclature in the ReflectionInfo stems from it being specific to ELF. If we want this to be limited to ELF, I think that we should be putting ELF into the name of the types (assuming that we will want custom types for MachO, WASM, COFF). I think that the types here are pretty generic though. I think that something like swift_reflection_address_mapping works well as this maps the local address to the remote address.

compnerd · 2020-07-15T15:49:58Z

include/swift/SwiftRemoteMirror/SwiftRemoteMirrorTypes.h

@@ -71,6 +82,17 @@ typedef struct swift_reflection_info {
  swift_reflection_ptr_t RemoteStartAddress;
 } swift_reflection_info_t;

+/// Represents the set of Swift reflection sections of an image,
+/// but doesn't assume that the sections share contiguous memory.


There is no real guarantee that reflection is contiguous. I think that it is sufficient to say:

Represents the Swift reflection metadata from an image.

I wanted to emphasize that this data structure doesn't assume the memory isn't contiguous to contrast the already existing swift_reflection_info, which does assume that. Should I remove that?

I think that the clarification that was made further up is critical here. It isn't that the reflection info is non-contiguous. The metadata layout is contiguous. The mapping from local to remote is non-contiguous. Is that understanding incorrect? If not, this is misleading, and I would say that this is better named as perhaps swift_remote_reflection_data_slice to indicate that this is a sliver of the full metadata information.

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

compnerd · 2020-07-15T15:52:42Z

stdlib/public/core/Misc.swift

+@_silgen_name("swift_getMetadataSectionName")
+public func _getMetadataSectionName(
+        _ metadata_section: UnsafeRawPointer)
+                -> UnsafePointer<CChar>


These don't seem to be following the standard library style.

Are these declarations conditionalized to when we're building a debug runtime that contains the symbols?

@jckarter I just remembered why they aren't conditionalized here. The problem is that SwiftReflectionTest.swift is built even when assertions are off. To conditionalize them here, I'd have to change the CMake config to only build this file in debug or on Mac. I initally did that, but was asked to roll-back that change. These functions are conditionalized in C++ though, so I believe calling them would crash the program, which isn't ideal.
In order to conditionalize these symbols, in SwiftReflectionTest.swift, when on Linux but not in debug, stub the required functions so it as least compiles. Is this solution ok?

What was wrong with conditionalizing SwiftReflectionTest.swift? If touching CMake is inappropriate, maybe we could #if out the contents.

stdlib/public/runtime/ImageInspectionCommon.cpp

jckarter · 2020-07-15T18:35:21Z

validation-test/Reflection/reflect_UInt16.swift

@@ -4,7 +4,7 @@

 // RUN: %target-run %target-swift-reflection-test %t/reflect_UInt16 | %FileCheck %s --check-prefix=CHECK-%target-ptrsize

-// REQUIRES: objc_interop
+// REQUIRES: basic_reflection_support


Should the condition be called reflection_test_support or something more specific?

@vedantk what do you think? I'm ok with either.

'reflection_test_support' sounds better than 'basic_reflection_support' to me, since it's not clear what the basic/non-basic distinction is.

augusto2112 · 2020-07-17T18:00:49Z

@jckarter @compnerd I've updated the new data-structures and function names:

swift_local_and_remote_reflection_section to swift_reflection_section_mapping
swift_non_contiguous_reflection_info to swift_reflection_mapping_info
swift_reflection_addNonContiguousReflectionInfo to swift_reflection_addReflectionMappingInfo.

Are these names all right?

compnerd · 2020-07-23T14:51:08Z

Thanks for the updates, those look way better.

jckarter

Looks great, thanks!

vedantk · 2020-07-23T21:24:45Z

@swift-ci smoke test and merge

vedantk · 2020-07-23T21:26:22Z

@swift-ci smoke test

vedantk · 2020-07-23T23:20:18Z

@augusto2112 it appears that availability markup is now required for these:

+Func _getMetadataSection(_:) is a new API without @available attribute
+Func _getMetadataSectionCount() is a new API without @available attribute
+Func _getMetadataSectionName(_:) is a new API without @available attribute

(either that, or the list in test/api-digester/stability-stdlib-abi-with-asserts.test can be updated). The new requirement came into effect on 7/17 with a8c785b.

https://ci.swift.org/job/swift-PR-osx-smoke-test/23940/consoleText

augusto2112 · 2020-07-24T17:50:13Z

@vedantk I see. Since these functions should only be available in debug builds, should we mark it with 9999? E.g.:

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)

vedantk · 2020-07-24T18:06:06Z

Instead of applying a fake at-available attribute, maybe it'd be simpler to add exceptions to the test/api-digester/stability-stdlib-abi-with-asserts.test test?

…flection tests on Linux

augusto2112 · 2020-07-24T19:17:25Z

@vedantk My mistake, I was looking at the old stability-stdlib-abi-with-asserts.swift file!

I've updated the pr, could you re-run the tests please?

vedantk · 2020-07-24T19:17:51Z

@swift-ci smoke test

adrian-prantl · 2020-07-25T01:13:38Z

🎉

lorentey · 2020-07-27T21:55:18Z

Instead of applying a fake at-available attribute, maybe it'd be simpler to add exceptions to the test/api-digester/stability-stdlib-abi-with-asserts.test test?

9999-availability isn't fake -- it's a placeholder for whatever stdlib release will first include these entry points. Without availability declarations, adopters will not be able to link with earlier stdlib releases.

theblixguy requested a review from mikeash June 12, 2020 01:19

augusto2112 force-pushed the swift-reflection-test-linux branch from e92f1af to 8dd2754 Compare June 12, 2020 20:11

vedantk changed the title ~~Adds entry points to the runtime that exposes metadata~~ WIP: Adds entry points to the runtime that exposes metadata Jun 12, 2020

augusto2112 force-pushed the swift-reflection-test-linux branch from 8dd2754 to 3598e7d Compare June 14, 2020 15:06

augusto2112 force-pushed the swift-reflection-test-linux branch 5 times, most recently from fab79de to 86545d9 Compare June 15, 2020 16:04

augusto2112 commented Jun 15, 2020

View reviewed changes

include/swift/Remote/MetadataReader.h Outdated Show resolved Hide resolved

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift Outdated Show resolved Hide resolved

vedantk reviewed Jun 15, 2020

View reviewed changes

augusto2112 force-pushed the swift-reflection-test-linux branch from 86545d9 to 528293d Compare June 15, 2020 21:51

vedantk reviewed Jun 15, 2020

View reviewed changes

augusto2112 force-pushed the swift-reflection-test-linux branch from 528293d to 66e903a Compare June 16, 2020 00:24

compnerd reviewed Jul 15, 2020

View reviewed changes

jckarter reviewed Jul 15, 2020

View reviewed changes

augusto2112 force-pushed the swift-reflection-test-linux branch from b2e9c37 to 12a801e Compare July 17, 2020 17:55

augusto2112 requested review from jckarter and compnerd July 17, 2020 17:57

compnerd approved these changes Jul 23, 2020

View reviewed changes

jckarter approved these changes Jul 23, 2020

View reviewed changes

augusto2112 force-pushed the swift-reflection-test-linux branch 2 times, most recently from 742cb09 to 3bcf957 Compare July 24, 2020 19:13

Add entrypoints to the runtime that exposes metadata necessary for re…

5016b3f

…flection tests on Linux

augusto2112 force-pushed the swift-reflection-test-linux branch from 3bcf957 to 5016b3f Compare July 24, 2020 19:15

vedantk merged commit 3cb8f9b into swiftlang:master Jul 24, 2020

This was referenced Jul 27, 2020

Enable validation reflection tests passing on Linux #33134

Merged

Conditionalize 'num_extra_inhabitants' in 64 bit platforms's tests #33136

Merged

natecook1000 mentioned this pull request Jul 27, 2020

[stdlib] Only include _getMetadataSection functions in stdlib builds w/ assertions #33142

Merged

Adds entry points to the runtime that exposes metadata #32339

Adds entry points to the runtime that exposes metadata #32339

Uh oh!

Conversation

augusto2112 commented Jun 12, 2020

Uh oh!

vedantk commented Jun 12, 2020

Uh oh!

augusto2112 commented Jun 12, 2020

Uh oh!

vedantk commented Jun 13, 2020

Uh oh!

vedantk commented Jun 13, 2020

Uh oh!

augusto2112 commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-prantl commented Jun 15, 2020

Uh oh!

adrian-prantl commented Jun 15, 2020

Uh oh!

augusto2112 commented Jun 15, 2020

Uh oh!

augusto2112 commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl commented Jun 15, 2020

Uh oh!

augusto2112 commented Jun 15, 2020

Uh oh!

adrian-prantl commented Jun 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vedantk commented Jun 15, 2020

Uh oh!

augusto2112 commented Jun 15, 2020

Uh oh!

vedantk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augusto2112 commented Jun 15, 2020

Uh oh!

vedantk commented Jun 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

augusto2112 commented Jun 14, 2020 •

edited

Loading

augusto2112 commented Jun 15, 2020 •

edited

Loading

compnerd Jul 16, 2020 •

edited

Loading