Skip to content

Commit e6f3dec

Browse files
tlivelytlively
authored
[Custom Descriptors] Fix accidental placeholder use in GTO (#7914)
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.
1 parent 50339e1 commit e6f3dec

3 files changed

Lines changed: 172 additions & 79 deletions

File tree

‎src/passes/GlobalTypeOptimization.cpp‎

Lines changed: 50 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
#include "ir/subtypes.h"
3232
#include "ir/type-updating.h"
3333
#include "pass.h"
34-
#include "support/insert_ordered.h"
3534
#include "support/permutations.h"
3635
#include "wasm-type-ordering.h"
3736
#include "wasm-type.h"
@@ -141,7 +140,7 @@ struct GlobalTypeOptimization : public Pass {
141140
// still need to be descriptors for their own subtypes and supertypes to be
142141
// valid. We will keep them descriptors by having them describe trivial new
143142
// placeholder types.
144-
std::vector<HeapType> descriptorsOfPlaceholders;
143+
std::unordered_set<HeapType> descriptorsOfPlaceholders;
145144

146145
void run(Module* module) override {
147146
if (!module->features.hasGC()) {
@@ -465,15 +464,13 @@ struct GlobalTypeOptimization : public Pass {
465464
descPropagator.propagateToSuperAndSubTypes(remainingDesciptors);
466465

467466
// Determine the set of descriptor types that will need placeholder
468-
// describees. Do not iterate directly on remainingDescriptors because it
469-
// is not deterministically ordered.
470-
for (auto type : subTypes.types) {
471-
if (auto it = remainingDesciptors.find(type);
472-
it != remainingDesciptors.end() && it->second.value) {
467+
// describees.
468+
for (auto [type, kept] : remainingDesciptors) {
469+
if (kept.value) {
473470
auto desc = type.getDescribedType();
474471
assert(desc);
475472
if (haveUnneededDescriptors.count(*desc)) {
476-
descriptorsOfPlaceholders.push_back(type);
473+
descriptorsOfPlaceholders.insert(type);
477474
}
478475
}
479476
}
@@ -497,31 +494,39 @@ struct GlobalTypeOptimization : public Pass {
497494
void updateTypes(Module& wasm) {
498495
class TypeRewriter : public GlobalTypeRewriter {
499496
GlobalTypeOptimization& parent;
500-
InsertOrderedMap<HeapType, Index> placeholderIndices;
501-
InsertOrderedMap<HeapType, Index>::iterator placeholderIt;
497+
// Differentiate the first occurrence of a descriptor of a placeholder
498+
// type, which is actually saving space for the placeholder itself, and
499+
// the next occurrence, which is real.
500+
bool sawPlaceholder = false;
502501

503502
public:
504503
TypeRewriter(Module& wasm, GlobalTypeOptimization& parent)
505-
: GlobalTypeRewriter(wasm), parent(parent),
506-
placeholderIt(placeholderIndices.begin()) {}
504+
: GlobalTypeRewriter(wasm), parent(parent) {}
507505

508506
std::vector<HeapType> getSortedTypes(PredecessorGraph preds) override {
509507
auto types = GlobalTypeRewriter::getSortedTypes(std::move(preds));
510-
// Some of the descriptors of placeholders may not end up being used at
511-
// all, so we will not rebuild them. Record the used types that need
512-
// placeholder describees and assign them placeholder indices.
513-
std::unordered_set<HeapType> typeSet(types.begin(), types.end());
514-
for (auto desc : parent.descriptorsOfPlaceholders) {
515-
if (typeSet.count(desc)) {
516-
placeholderIndices.insert({desc, placeholderIndices.size()});
508+
// Intersperse placeholder types throughout the sorted types. Each
509+
// descriptor type that needs a placeholder describee will be duplicated
510+
// in the list of sorted types. The first appearance will be modified to
511+
// become the placeholder describee and the subsequent appearance will
512+
// be modified to describe the preceding placeholder. We represent the
513+
// placeholder slots here using their intended descriptor types for two
514+
// reasons: first, it lets us easily recognize the placeholder slots
515+
// in modifyTypeBuilderEntry. Second, it ensures that the type-to-index
516+
// mapping created in GlobalTypeRewriter::rebuiltTypes does not end up
517+
// mapping the placeholder types to any index, since their mappings are
518+
// immediately overwritten by the following "real" occurrences of the
519+
// descriptor types.
520+
std::vector<HeapType> typesWithPlaceholders;
521+
for (auto type : types) {
522+
if (parent.descriptorsOfPlaceholders.count(type)) {
523+
// Insert the type an extra time to make space for the placeholder.
524+
typesWithPlaceholders.push_back(type);
517525
}
526+
typesWithPlaceholders.push_back(type);
518527
}
519-
placeholderIt = placeholderIndices.begin();
520-
// Prefix the types with placeholders to be overwritten with the
521-
// placeholder describees.
522-
HeapType placeholder = Struct{};
523-
types.insert(types.begin(), placeholderIndices.size(), placeholder);
524-
return types;
528+
529+
return typesWithPlaceholders;
525530
}
526531

527532
void modifyStruct(HeapType oldStructType, Struct& struct_) override {
@@ -584,33 +589,36 @@ struct GlobalTypeOptimization : public Pass {
584589
return;
585590
}
586591

587-
// Until we've created all the placeholders, create a placeholder
588-
// describee type for the next descriptor that needs one.
589-
if (placeholderIt != placeholderIndices.end()) {
590-
auto descriptor = placeholderIt->first;
591-
typeBuilder[i].descriptor(getTempHeapType(descriptor));
592-
typeBuilder[i].setShared(descriptor.getShared());
593-
594-
++placeholderIt;
595-
return;
592+
// Remove an unneeded descriptor.
593+
if (parent.haveUnneededDescriptors.count(oldType)) {
594+
typeBuilder[i].descriptor(std::nullopt);
596595
}
597596

598597
// Remove an unneeded describee or describe a placeholder type.
599598
if (auto described = oldType.getDescribedType()) {
600599
if (parent.haveUnneededDescriptors.count(*described)) {
601-
if (auto it = placeholderIndices.find(oldType);
602-
it != placeholderIndices.end()) {
603-
typeBuilder[i].describes(typeBuilder[it->second]);
600+
if (parent.descriptorsOfPlaceholders.count(oldType)) {
601+
if (!sawPlaceholder) {
602+
// This is the placeholder describee. Set its descriptor to be
603+
// the succeeding real descriptor type.
604+
typeBuilder[i] = Struct{};
605+
typeBuilder[i].setShared(described->getShared());
606+
typeBuilder[i].descriptor(typeBuilder[i + 1]);
607+
typeBuilder[i].describes(std::nullopt);
608+
typeBuilder[i].subTypeOf(std::nullopt);
609+
sawPlaceholder = true;
610+
} else {
611+
// This is the real placedholder-describing descriptor type.
612+
// Have it describe the preceding placeholder describee.
613+
typeBuilder[i].describes(typeBuilder[i - 1]);
614+
sawPlaceholder = false;
615+
}
604616
} else {
617+
// This type no longer needs its describee.
605618
typeBuilder[i].describes(std::nullopt);
606619
}
607620
}
608621
}
609-
610-
// Remove an unneeded descriptor.
611-
if (parent.haveUnneededDescriptors.count(oldType)) {
612-
typeBuilder.setDescriptor(i, std::nullopt);
613-
}
614622
}
615623
};
616624

‎test/lit/passes/gto-desc-tnh.wast‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -103,13 +103,13 @@
103103
(module
104104
(rec
105105
;; CHECK: (rec
106-
;; CHECK-NEXT: (type $0 (descriptor $A.desc (struct)))
107-
108-
;; CHECK: (type $A (sub (struct)))
106+
;; CHECK-NEXT: (type $A (sub (struct)))
109107
;; T_N_H: (rec
110108
;; T_N_H-NEXT: (type $A (sub (struct)))
111109
(type $A (sub (descriptor $A.desc (struct))))
112-
;; CHECK: (type $A.desc (sub (describes $0 (struct))))
110+
;; CHECK: (type $1 (sub (descriptor $A.desc (struct))))
111+
112+
;; CHECK: (type $A.desc (sub (describes $1 (struct))))
113113
;; T_N_H: (type $A.desc (sub (struct)))
114114
(type $A.desc (sub (describes $A (struct ))))
115115

@@ -155,13 +155,9 @@
155155
(module
156156
(rec
157157
;; CHECK: (rec
158-
;; CHECK-NEXT: (type $0 (descriptor $B.desc (struct)))
159-
160-
;; CHECK: (type $A (sub (descriptor $A.desc (struct))))
158+
;; CHECK-NEXT: (type $A (sub (descriptor $A.desc (struct))))
161159
;; T_N_H: (rec
162-
;; T_N_H-NEXT: (type $0 (descriptor $B.desc (struct)))
163-
164-
;; T_N_H: (type $A (sub (descriptor $A.desc (struct))))
160+
;; T_N_H-NEXT: (type $A (sub (descriptor $A.desc (struct))))
165161
(type $A (sub (descriptor $A.desc (struct))))
166162
;; CHECK: (type $A.desc (sub (describes $A (struct))))
167163
;; T_N_H: (type $A.desc (sub (describes $A (struct))))
@@ -170,8 +166,12 @@
170166
;; CHECK: (type $B (sub (struct)))
171167
;; T_N_H: (type $B (sub (struct)))
172168
(type $B (sub (descriptor $B.desc (struct))))
173-
;; CHECK: (type $B.desc (sub $A.desc (describes $0 (struct))))
174-
;; T_N_H: (type $B.desc (sub $A.desc (describes $0 (struct))))
169+
;; CHECK: (type $3 (sub (descriptor $B.desc (struct))))
170+
171+
;; CHECK: (type $B.desc (sub $A.desc (describes $3 (struct))))
172+
;; T_N_H: (type $3 (sub (descriptor $B.desc (struct))))
173+
174+
;; T_N_H: (type $B.desc (sub $A.desc (describes $3 (struct))))
175175
(type $B.desc (sub $A.desc (describes $B (struct))))
176176
)
177177

‎test/lit/passes/gto-desc.wast‎

Lines changed: 110 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -869,11 +869,11 @@
869869
(module
870870
(rec
871871
;; CHECK: (rec
872-
;; CHECK-NEXT: (type $0 (descriptor $A.desc (struct)))
873-
874-
;; CHECK: (type $A (sub (struct)))
872+
;; CHECK-NEXT: (type $A (sub (struct)))
875873
(type $A (sub (descriptor $A.desc (struct))))
876-
;; CHECK: (type $A.desc (sub (describes $0 (struct))))
874+
;; CHECK: (type $1 (sub (descriptor $A.desc (struct))))
875+
876+
;; CHECK: (type $A.desc (sub (describes $1 (struct))))
877877
(type $A.desc (sub (describes $A (struct))))
878878

879879
;; CHECK: (type $B (sub $A (descriptor $B.desc (struct))))
@@ -950,18 +950,18 @@
950950
(module
951951
(rec
952952
;; CHECK: (rec
953-
;; CHECK-NEXT: (type $0 (descriptor $A.desc (struct)))
954-
955-
;; CHECK: (type $1 (descriptor $B.desc (struct)))
956-
957-
;; CHECK: (type $A (sub (struct)))
953+
;; CHECK-NEXT: (type $A (sub (struct)))
958954
(type $A (sub (descriptor $A.desc (struct))))
959-
;; CHECK: (type $A.desc (sub (describes $0 (struct))))
955+
;; CHECK: (type $1 (sub (descriptor $A.desc (struct))))
956+
957+
;; CHECK: (type $A.desc (sub (describes $1 (struct))))
960958
(type $A.desc (sub (describes $A (struct))))
961959

962960
;; CHECK: (type $B (sub $A (struct)))
963961
(type $B (sub $A (descriptor $B.desc (struct))))
964-
;; CHECK: (type $B.desc (sub $A.desc (describes $1 (struct))))
962+
;; CHECK: (type $4 (sub (descriptor $B.desc (struct))))
963+
964+
;; CHECK: (type $B.desc (sub $A.desc (describes $4 (struct))))
965965
(type $B.desc (sub $A.desc (describes $B (struct))))
966966

967967
;; CHECK: (type $C (sub $B (descriptor $C.desc (struct))))
@@ -1079,11 +1079,11 @@
10791079
)
10801080
(rec
10811081
;; CHECK: (rec
1082-
;; CHECK-NEXT: (type $1 (descriptor $unused.desc (struct)))
1083-
1084-
;; CHECK: (type $unused (sub $public (struct)))
1082+
;; CHECK-NEXT: (type $unused (sub $public (struct)))
10851083
(type $unused (sub $public (descriptor $unused.desc (struct))))
1086-
;; CHECK: (type $unused.desc (sub (describes $1 (struct))))
1084+
;; CHECK: (type $2 (sub (descriptor $unused.desc (struct))))
1085+
1086+
;; CHECK: (type $unused.desc (sub (describes $2 (struct))))
10871087
(type $unused.desc (sub (describes $unused (struct))))
10881088

10891089
;; CHECK: (type $used (sub $unused (descriptor $used.desc (struct))))
@@ -1116,11 +1116,11 @@
11161116
(module
11171117
(rec
11181118
;; CHECK: (rec
1119-
;; CHECK-NEXT: (type $0 (shared (descriptor $A.desc (struct))))
1120-
1121-
;; CHECK: (type $A (sub (shared (struct))))
1119+
;; CHECK-NEXT: (type $A (sub (shared (struct))))
11221120
(type $A (sub (shared (descriptor $A.desc (struct)))))
1123-
;; CHECK: (type $A.desc (sub (shared (describes $0 (struct)))))
1121+
;; CHECK: (type $1 (sub (shared (descriptor $A.desc (struct)))))
1122+
1123+
;; CHECK: (type $A.desc (sub (shared (describes $1 (struct)))))
11241124
(type $A.desc (sub (shared (describes $A (struct)))))
11251125

11261126
;; CHECK: (type $B (sub $A (shared (descriptor $B.desc (struct)))))
@@ -1176,23 +1176,108 @@
11761176
;; Here we would optimize $unused with a placeholder, except that we don't
11771177
;; include it in the output at all because it is unused. We should not get
11781178
;; confused and try to emit a placeholder for it anyway.
1179+
(module
1180+
(rec
1181+
;; CHECK: (rec
1182+
;; CHECK-NEXT: (type $public (sub (descriptor $public.desc (struct))))
1183+
(type $public (sub (descriptor $public.desc (struct))))
1184+
;; CHECK: (type $public.desc (sub (describes $public (struct))))
1185+
(type $public.desc (sub (describes $public (struct))))
1186+
)
1187+
(rec
1188+
(type $unused (descriptor $unused.desc (struct)))
1189+
(type $unused.desc (sub $public.desc (describes $unused (struct))))
1190+
;; CHECK: (type $private (struct))
1191+
(type $private (struct))
1192+
)
1193+
1194+
;; CHECK: (global $public (ref null $public) (ref.null none))
1195+
(global $public (export "public") (ref null $public) (ref.null none))
1196+
;; CHECK: (global $private (ref null $private) (ref.null none))
1197+
(global $private (ref null $private) (ref.null none))
1198+
)
1199+
1200+
;; Regression test for a bug where we accidentally replaced empty struct types
1201+
;; with placeholders.
1202+
;; CHECK: (export "public" (global $public))
1203+
(module
1204+
;; CHECK: (type $public (struct))
1205+
(type $public (struct))
1206+
(rec
1207+
;; CHECK: (rec
1208+
;; CHECK-NEXT: (type $super (sub (struct)))
1209+
(type $super (sub (descriptor $super.desc (struct))))
1210+
;; CHECK: (type $2 (sub (descriptor $super.desc (struct))))
1211+
1212+
;; CHECK: (type $super.desc (sub (describes $2 (struct))))
1213+
(type $super.desc (sub (describes $super (struct))))
1214+
;; CHECK: (type $sub (sub $super (descriptor $sub.desc (struct))))
1215+
(type $sub (sub $super (descriptor $sub.desc (struct))))
1216+
;; CHECK: (type $sub.desc (sub $super.desc (describes $sub (struct))))
1217+
(type $sub.desc (sub $super.desc (describes $sub (struct))))
1218+
)
1219+
1220+
;; CHECK: (type $6 (func (param (ref $sub))))
1221+
1222+
;; CHECK: (global $public (ref null $public) (ref.null none))
1223+
(global $public (export "public") (ref null $public) (ref.null none))
1224+
1225+
;; CHECK: (export "public" (global $public))
1226+
1227+
;; CHECK: (func $test (type $6) (param $sub (ref $sub))
1228+
;; CHECK-NEXT: (drop
1229+
;; CHECK-NEXT: (ref.get_desc $sub
1230+
;; CHECK-NEXT: (local.get $sub)
1231+
;; CHECK-NEXT: )
1232+
;; CHECK-NEXT: )
1233+
;; CHECK-NEXT: (drop
1234+
;; CHECK-NEXT: (struct.new_default $public)
1235+
;; CHECK-NEXT: )
1236+
;; CHECK-NEXT: )
1237+
(func $test (param $sub (ref $sub))
1238+
;; Use $sub's descriptor to keep it a descriptor. This forces $super.desc to
1239+
;; describe a placeholder.
1240+
(drop
1241+
(ref.get_desc $sub
1242+
(local.get $sub)
1243+
)
1244+
)
1245+
;; We should not accidentally replace this trivial struct with the
1246+
;; placeholder, which would cause a validation failure because we do not
1247+
;; supply a descriptor in this allocation.
1248+
(drop
1249+
(struct.new_default $public)
1250+
)
1251+
)
1252+
)
1253+
1254+
;; Regression test for a bug where a placeholder was not set up to be described
1255+
;; by its intended descriptor if that intended descriptor also happened to have
1256+
;; its own unneeded descriptor.
11791257
(module
11801258
(rec
11811259
;; CHECK: (rec
1182-
;; CHECK-NEXT: (type $public (sub (descriptor $public.desc (struct))))
1183-
(type $public (sub (descriptor $public.desc (struct))))
1260+
;; CHECK-NEXT: (type $public (descriptor $public.desc (struct)))
1261+
(type $public (descriptor $public.desc (struct)))
11841262
;; CHECK: (type $public.desc (sub (describes $public (struct))))
11851263
(type $public.desc (sub (describes $public (struct))))
11861264
)
11871265
(rec
1188-
(type $unused (descriptor $unused.desc (struct)))
1189-
(type $unused.desc (sub $public.desc (describes $unused (struct))))
1190-
;; CHECK: (type $private (struct))
1191-
(type $private (struct))
1266+
;; CHECK: (rec
1267+
;; CHECK-NEXT: (type $private (struct))
1268+
(type $private (descriptor $private.desc (struct)))
1269+
;; CHECK: (type $3 (sub (descriptor $private.desc (struct))))
1270+
1271+
;; CHECK: (type $private.desc (sub $public.desc (describes $3 (struct))))
1272+
(type $private.desc (sub $public.desc (describes $private (descriptor $private.meta (struct)))))
1273+
;; CHECK: (type $private.meta (struct))
1274+
(type $private.meta (describes $private.desc (struct)))
11921275
)
11931276

1277+
;; Force $private.desc to remain a descriptor.
11941278
;; CHECK: (global $public (ref null $public) (ref.null none))
11951279
(global $public (export "public") (ref null $public) (ref.null none))
1280+
11961281
;; CHECK: (global $private (ref null $private) (ref.null none))
11971282
(global $private (ref null $private) (ref.null none))
11981283
)

0 commit comments

Comments
 (0)