Skip to content

[IRGen] fix crashes emitting calls to get metadata from mangled name (16 bit architectures) #70957

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

Conversation

carlos4242
Copy link
Contributor

This tries to generalise the TypeMetadataDemanglingCacheVariable. In all other architectures this will emit an (i32, i32). For 16 bit architectures, the relative address should be an i16 to get the right relocations/fixups.

Also adjusts the generated __swift_instantiateConcreteTypeFromMangledName code emission to allow for the fact that SizeTy on 16 bit architectures is smaller than i32.

@kubamracek
Copy link
Contributor

@carlos4242 Is this relevant in Embedded Swift? The metadata caches should be unused (or never emitted in the first place) in Embedded Swift.

@carlos4242
Copy link
Contributor Author

carlos4242 commented Jan 19, 2024

Fair comment. This is an older bug fix that was necessary for us at the time. Because we couldn’t disable metadata back then. So we had to work around it (it was unused in our simple programs and got culled in linker garbage collection) but emission couldn’t crash or our toolchain broke.

Four our live product (5.3.5) the compiler still needs this patch because we have not yet got embedded mode live.

Embedded mode is in our 6.0 beta IDE but it non trivially breaks a number of parts of our current standard library (mostly our rather specialised Arrays) and runtime and the fixes may need some help from apple engineering and are unlikely to be done soon.

So we can leave this patch only on our fork on the reasoning that all 16-bit platforms (a rare niche) will never want metadata in this way… which is reasonable.

Or we can move this into the mainline on the thinking that it is just an extension of my previous mainline compiler patch for relative addressing on 16-bit pointers … which is also reasonable.

@kubamracek
Copy link
Contributor

I'm not against making fixes for non-embedded Swift, I'm just wondering about the details -- are you saying that the demangling cache globals are actually used at runtime? I had the impression that you don't use a runtime type instantiation and the type registry... I guess that's not the case?

@carlos4242
Copy link
Contributor Author

This bug fix has been there for a few years. It's the oldest patch I've submitted so far... I can't remember the exact crashes that were happening without it.

To be clear, in all the code I've seen on our platform, the demangling cache globals are NEVER used at runtime. However from what I remember, the compiler was still trying to emit them sometimes. When this code was lowered down into the llvm layers I think that's where the problems started to happen. From what I remember it was down to relocations. It was either that LLVM couldn't emit compatible relocations or that it emitted relocations that the AVR linker couldn't link. But basically the bottom line was you were trying to create relocations forcing a (minimum) i32 sized type instead of using the actual address pointer type for the platform (which is i16 on AVR). Since AVR is the only 16 bit platform for Swift (except maybe Jordan Rose's experiments??) ... it's only ever been an issue for me.

It might be easier to just leave this patch out. It's fine on our branch, we'll keep it until it's no longer relevant. (My plan is that our compiler becomes embedded mode only in due course.)

@carlos4242
Copy link
Contributor Author

@kubamracek ... for the sake of keeping things simple, I'm thinking maybe I should just close this PR? There are more important things I'd like to upstream and I'm tempted to say once AVR goes pure embedded mode, it might not be an issue anyway. Probably easier to close this then reopen a PR later if this comes back. OK with you?

@carlos4242
Copy link
Contributor Author

abandoned in favour of #74693

@carlos4242 carlos4242 closed this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants