[Custom Descriptors] Fix accidental placeholder use in GTO#7914
Merged
[Custom Descriptors] Fix accidental placeholder use in GTO#7914
Conversation
GTO inserts placeholder describee types by overriding getSortedTypes and returning a list of types with extra structs added where the placeholders will go. Previously we used the empty struct to reserve space in the list for the placeholders, but when the empty struct was a public type (and therefore did not appear after the placeholders in the sorted list of rebuilt types), this had the unfortunate effect of replacing the empty struct with one of the placeholders throughout the module. This did not happen when the emptry struct was a private type because then it would appear later in the list of sorted types and its type-to-index mapping in GlobalTypeRewriter::rebuildTypes would be updated to map to the non-placeholder index. When the type is public, this later entry does not exist and the type ends up being mapped to the placeholder index. To fix the problem, change how we save space for placeholder describees in the list of sorted types to ensure the placeholder is represented by a private type that will appear later in the list of types. This will ensure that the final mapping of types to indices will always map types to the index of their "real" appearance rather than the index of a placeholder type, and will also ensure that public types are never mapped to anything else. Specifically, we now save space for a described type using the descriptor type that will eventually describe it. While refactoring this, also simplify things by placing the placeholder describee types directly before their descriptor types in the sorted list. This saves us from having to map from descriptor types to the index of their placeholders since we can determine the placeholder's index from its descriptor's index.
kripken
approved these changes
Sep 23, 2025
| GlobalTypeOptimization& parent; | ||
| InsertOrderedMap<HeapType, Index> placeholderIndices; | ||
| InsertOrderedMap<HeapType, Index>::iterator placeholderIt; | ||
| bool sawPlaceholder = false; |
Member
There was a problem hiding this comment.
Worth a comment for this cycling value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GTO inserts placeholder describee types by overriding getSortedTypes
and returning a list of types with extra structs added where the
placeholders will go. Previously we used the empty struct to reserve
space in the list for the placeholders, but when the empty struct was a
public type (and therefore did not appear after the placeholders in the
sorted list of rebuilt types), this had the unfortunate effect of
replacing the empty struct with one of the placeholders throughout the
module. This did not happen when the emptry struct was a private type
because then it would appear later in the list of sorted types and its
type-to-index mapping in GlobalTypeRewriter::rebuildTypes would be
updated to map to the non-placeholder index. When the type is public,
this later entry does not exist and the type ends up being mapped to the
placeholder index.
To fix the problem, change how we save space for placeholder describees
in the list of sorted types to ensure the placeholder is represented by
a private type that will appear later in the list of types. This will
ensure that the final mapping of types to indices will always map types
to the index of their "real" appearance rather than the index of a
placeholder type, and will also ensure that public types are never
mapped to anything else. Specifically, we now save space for a described
type using the descriptor type that will eventually describe it.
While refactoring this, also simplify things by placing the placeholder
describee types directly before their descriptor types in the sorted
list. This saves us from having to map from descriptor types to the
index of their placeholders since we can determine the placeholder's
index from its descriptor's index.