Skip to content

Commit 2138991

Browse files
stookestooke
committed
Rework MethodList builder, other changes from review.
1 parent ba6b72d commit 2138991

File tree

2 files changed

+63
-72
lines changed

2 files changed

+63
-72
lines changed

‎substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java‎

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ int computeFullContents(byte[] buffer, int initialPos) {
108108
pos = alignPadded4(buffer, pos);
109109
int length = pos - initialPos - Short.BYTES;
110110
if (length > CV_TYPE_RECORD_MAX_SIZE) {
111-
GraalError.shouldNotReachHere(String.format("Type record too large: %d (maximum %d) bytes: %s", length, CV_TYPE_RECORD_MAX_SIZE, this));
111+
throw GraalError.shouldNotReachHere(String.format("Type record too large: %d (maximum %d) bytes: %s", length, CV_TYPE_RECORD_MAX_SIZE, this));
112112
}
113113
CVUtil.putShort((short) length, buffer, initialPos);
114114
return pos;
@@ -617,9 +617,7 @@ protected FieldRecord(short leafType, short attrs, String name) {
617617
}
618618

619619
protected FieldRecord(short leafType) {
620-
this.type = leafType;
621-
this.attrs = 0;
622-
this.name = "";
620+
this(leafType, (short) 0, "");
623621
}
624622

625623
public int computeSize() {
@@ -638,42 +636,41 @@ public int hashCode() {
638636

639637
@Override
640638
public boolean equals(Object obj) {
641-
if (!super.equals(obj)) {
642-
return false;
643-
}
644639
FieldRecord other = (FieldRecord) obj;
645-
return this.hashCode() == other.hashCode();
640+
return this.type == other.type && this.attrs == other.attrs && this.name.equals(other.name);
646641
}
647642
}
648643

649644
static final class CVOverloadedMethodRecord extends FieldRecord {
650645

651646
private final int methodListIndex; /* index of method list record */
647+
private final short count;
652648

653649
CVOverloadedMethodRecord(short count, int methodListIndex, String methodName) {
654-
/* Stick 'count' into the attrs field just for convenience. */
655-
super(LF_METHOD, count, methodName);
650+
super(LF_METHOD, (short) 0, methodName);
656651
this.methodListIndex = methodListIndex;
652+
this.count = count;
657653
}
658654

659655
@Override
660656
public int computeContents(byte[] buffer, int initialPos) {
661657
int pos = CVUtil.putShort(type, buffer, initialPos);
662-
pos = CVUtil.putShort(attrs, buffer, pos); /* Remember, this is actually 'count'. */
658+
pos = CVUtil.putShort(count, buffer, pos);
663659
pos = CVUtil.putInt(methodListIndex, buffer, pos);
664660
pos = CVUtil.putUTF8StringBytes(name, buffer, pos);
665661
return pos;
666662
}
667663

668664
@Override
669665
public String toString() {
670-
return String.format("LF_METHOD(0x%04x) count=0x%x listIdx=0x%04x %s", type, attrs, methodListIndex, name);
666+
return String.format("LF_METHOD(0x%04x) count=0x%x listIdx=0x%04x %s", type, count, methodListIndex, name);
671667
}
672668

673669
@Override
674670
public int hashCode() {
675671
int h = super.hashCode();
676672
h = 31 * h + methodListIndex;
673+
h = 31 + h + count;
677674
return h;
678675
}
679676

@@ -683,7 +680,7 @@ public boolean equals(Object obj) {
683680
return false;
684681
}
685682
CVOverloadedMethodRecord other = (CVOverloadedMethodRecord) obj;
686-
return this.methodListIndex == other.methodListIndex;
683+
return this.methodListIndex == other.methodListIndex && this.count == other.count;
687684
}
688685
}
689686

@@ -1037,7 +1034,9 @@ public boolean equals(Object obj) {
10371034
static final class CVFieldListRecord extends CVTypeRecord {
10381035

10391036
static final int INITIAL_CAPACITY = 10;
1040-
private int estimatedSize = 0;
1037+
1038+
/* Size includes type field but not record length field. */
1039+
private int estimatedSize = CVUtil.align4(Short.BYTES);
10411040

10421041
private final ArrayList<FieldRecord> members = new ArrayList<>(INITIAL_CAPACITY);
10431042

@@ -1047,29 +1046,19 @@ static final class CVFieldListRecord extends CVTypeRecord {
10471046

10481047
void add(FieldRecord m) {
10491048
/* Keep a running total. */
1050-
estimatedSize += m.computeSize();
1051-
/* Pad to next 4 byte boundary if required. */
1052-
while ((estimatedSize % 4) != 0) {
1053-
estimatedSize++;
1054-
}
1049+
estimatedSize += CVUtil.align4(m.computeSize());
10551050
members.add(m);
10561051
}
10571052

1058-
int count() {
1059-
return members.size();
1060-
}
1061-
10621053
int getEstimatedSize() {
1063-
/* Add in size of record type and length fields. */
1064-
return estimatedSize + Short.BYTES * 2;
1054+
return estimatedSize;
10651055
}
10661056

10671057
@Override
10681058
protected int computeContents(byte[] buffer, int initialPos) {
1069-
int pos = initialPos;
1059+
int pos = CVTypeRecord.alignPadded4(buffer, initialPos);
10701060
for (FieldRecord field : members) {
10711061
pos = field.computeContents(buffer, pos);
1072-
/* Align on 4-byte boundary. */
10731062
pos = CVTypeRecord.alignPadded4(buffer, pos);
10741063
}
10751064
return pos;
@@ -1126,11 +1115,6 @@ static final class CVTypeArrayRecord extends CVTypeRecord {
11261115
this(elementType.getSequenceNumber(), T_UINT8, length);
11271116
}
11281117

1129-
@Override
1130-
public int computeSize(int initialPos) {
1131-
return initialPos + Integer.BYTES * 3;
1132-
}
1133-
11341118
@Override
11351119
public int computeContents(byte[] buffer, int initialPos) {
11361120
int pos = CVUtil.putInt(elementTypeIndex, buffer, initialPos);

‎substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java‎

Lines changed: 47 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.graalvm.compiler.debug.GraalError;
4040

4141
import java.lang.reflect.Modifier;
42+
import java.util.ArrayList;
4243
import java.util.Collections;
4344
import java.util.Deque;
4445
import java.util.HashMap;
@@ -152,42 +153,52 @@ CVTypeRecord buildFunction(PrimaryEntry entry) {
152153
return buildMemberFunction(entry.getClassEntry(), entry.getPrimary().getMethodEntry());
153154
}
154155

155-
static class FieldListContext {
156-
/* Knock 100 bytes off to be safe from off-by-1, padding, LF_INDEX, etc. */
157-
static final int FIELD_LIST_MAX_SIZE = CV_TYPE_RECORD_MAX_SIZE - 100;
156+
static class FieldListBuilder {
157+
final List<CVTypeRecord.FieldRecord> fields = new ArrayList<>();
158158

159-
final CVTypeRecord.CVFieldListRecord firstFieldList;
160-
CVTypeRecord.CVFieldListRecord currentFieldList;
161-
final Deque<CVTypeRecord.CVFieldListRecord> lists = new LinkedList<>();
159+
FieldListBuilder() {
160+
}
162161

163-
FieldListContext(CVTypeRecord.CVFieldListRecord initialRecord) {
164-
this.firstFieldList = initialRecord;
165-
this.currentFieldList = initialRecord;
166-
lists.add(firstFieldList);
162+
void addField(CVTypeRecord.FieldRecord field) {
163+
fields.add(field);
167164
}
168165

169-
void addRecordToFieldList(CVTypeRecord.FieldRecord field) {
170-
if ((currentFieldList.getEstimatedSize() + field.computeSize()) > FIELD_LIST_MAX_SIZE) {
171-
/* end this fieldlist with an LF_INDEX to the new field list. */
172-
currentFieldList = new CVTypeRecord.CVFieldListRecord();
173-
lists.add(currentFieldList);
174-
}
175-
currentFieldList.add(field);
166+
int getFieldCount() {
167+
return fields.size();
176168
}
177169

178-
CVTypeRecord.CVFieldListRecord addTypeRecords(CVTypeSectionBuilder builder) {
179-
/* Add all fieldlist records in the reverse order they were created. */
170+
CVTypeRecord.CVFieldListRecord buildFieldListRecords(CVTypeSectionBuilder builder) {
171+
172+
/* The last FieldList must refer back to the one before it,
173+
and must contain the first fields in the class. */
174+
175+
CVTypeRecord.CVFieldListRecord currentFieldList = new CVTypeRecord.CVFieldListRecord();
176+
Deque<CVTypeRecord.CVFieldListRecord> fl = new LinkedList<>();
177+
fl.add(currentFieldList);
178+
179+
/* Build all Field List records in field order (FIFO). */
180+
for (CVTypeRecord.FieldRecord fieldRecord : fields) {
181+
if (currentFieldList.getEstimatedSize() + CVUtil.align4(fieldRecord.computeSize()) >= CV_TYPE_RECORD_MAX_SIZE) {
182+
currentFieldList = new CVTypeRecord.CVFieldListRecord();
183+
fl.add(currentFieldList);
184+
}
185+
currentFieldList.add(fieldRecord);
186+
}
187+
188+
/* Emit all Field List records in reverse order (LIFO), adding Index records
189+
to all but the first emitted. */
190+
CVTypeRecord.CVFieldListRecord fieldListRecord = null;
180191
int idx = 0;
181-
while (!lists.isEmpty()) {
182-
CVTypeRecord.CVFieldListRecord fieldList = lists.removeLast();
183-
currentFieldList = builder.addTypeRecord(fieldList);
192+
while (!fl.isEmpty()) {
193+
fieldListRecord = fl.removeLast();
194+
fieldListRecord = builder.addTypeRecord(fieldListRecord);
184195
/* For all fieldlist but the first, link to the previous record. */
185196
if (idx != 0) {
186-
fieldList.add(new CVTypeRecord.CVIndexRecord(idx));
197+
fieldListRecord.add(new CVTypeRecord.CVIndexRecord(idx));
187198
}
188-
idx = currentFieldList.getSequenceNumber();
199+
idx = fieldListRecord.getSequenceNumber();
189200
}
190-
return currentFieldList;
201+
return fieldListRecord;
191202
}
192203
}
193204

@@ -212,26 +223,22 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
212223

213224
final List<MethodEntry> methods = typeEntry.isClass() ? ((ClassEntry) typeEntry).getMethods() : Collections.emptyList();
214225

215-
/* process fields */
216-
217226
/* Build fieldlist record */
218-
CVTypeRecord.CVFieldListRecord fieldListRecord = new CVTypeRecord.CVFieldListRecord();
219-
FieldListContext fieldListContext = new FieldListContext(fieldListRecord);
220-
221-
log("building fieldlist %s", fieldListRecord);
227+
FieldListBuilder fieldListBuilder = new FieldListBuilder();
228+
log("building field list");
222229

223230
if (superTypeIndex != 0) {
224231
CVTypeRecord.CVBaseMemberRecord btype = new CVTypeRecord.CVBaseMemberRecord(MPROP_PUBLIC, superTypeIndex, 0);
225232
log("basetype %s", btype);
226-
fieldListContext.addRecordToFieldList(btype);
233+
fieldListBuilder.addField(btype);
227234
}
228235

229236
/* Only define manifested fields. */
230237
typeEntry.fields().filter(CVTypeSectionBuilder::isManifestedField).forEach(f -> {
231238
log("field %s attr=(%s) offset=%d size=%d valuetype=%s", f.fieldName(), f.getModifiersString(), f.getOffset(), f.getSize(), f.getValueType().getTypeName());
232239
CVTypeRecord.FieldRecord fieldRecord = buildField(f);
233240
log("field %s", fieldRecord);
234-
fieldListContext.addRecordToFieldList(fieldRecord);
241+
fieldListBuilder.addField(fieldRecord);
235242
});
236243

237244
if (typeEntry.isArray()) {
@@ -250,7 +257,7 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
250257
/* Build a field for the 0 length array. */
251258
CVTypeRecord.CVMemberRecord dm = new CVTypeRecord.CVMemberRecord(MPROP_PUBLIC, array0record.getSequenceNumber(), typeEntry.getSize(), "data");
252259
log("field %s", dm);
253-
fieldListContext.addRecordToFieldList(dm);
260+
fieldListBuilder.addField(dm);
254261
}
255262

256263
/*
@@ -292,25 +299,25 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
292299

293300
/* LF_METHOD record */
294301
CVTypeRecord.CVOverloadedMethodRecord methodRecord = new CVTypeRecord.CVOverloadedMethodRecord((short) nmlist.count(), nmlist.getSequenceNumber(), mname);
295-
fieldListContext.addRecordToFieldList(methodRecord);
302+
fieldListBuilder.addField(methodRecord);
296303
});
297304

298305
methods.stream().filter(methodEntry -> !overloaded.contains(methodEntry.methodName())).forEach(m -> {
299306
log("`unique method %s %s %s(...)", m.fieldName(), m.methodName(), m.getModifiersString(), m.getValueType().getTypeName(), m.methodName());
300307
CVTypeRecord.CVOneMethodRecord method = buildMethod((ClassEntry) typeEntry, m);
301308
log(" unique method %s", method);
302-
fieldListContext.addRecordToFieldList(method);
309+
fieldListBuilder.addField(method);
303310
});
304311
}
305312
/* Build fieldlist record from manifested fields. */
306-
CVTypeRecord.CVFieldListRecord newfieldListRecord = fieldListContext.addTypeRecords(this);
313+
CVTypeRecord.CVFieldListRecord newfieldListRecord = fieldListBuilder.buildFieldListRecords(this);
307314
int fieldListIdx = newfieldListRecord.getSequenceNumber();
308-
int fieldListCount = newfieldListRecord.count();
315+
int fieldCount = fieldListBuilder.getFieldCount();
309316
log("finished building fieldlist %s", newfieldListRecord);
310317

311318
/* Build final class record. */
312319
short attrs = 0; /* property attribute field (prop_t) */
313-
CVTypeRecord typeRecord = new CVTypeRecord.CVClassRecord(LF_CLASS, (short) fieldListCount, attrs, fieldListIdx, 0, 0, typeEntry.getSize(), typeEntry.getTypeName(), null);
320+
CVTypeRecord typeRecord = new CVTypeRecord.CVClassRecord(LF_CLASS, (short) fieldCount, attrs, fieldListIdx, 0, 0, typeEntry.getSize(), typeEntry.getTypeName(), null);
314321
typeRecord = addTypeRecord(typeRecord);
315322

316323
if (typeEntry.isClass()) {
@@ -330,7 +337,7 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
330337

331338
types.typeNameMap.put(typeEntry.getTypeName(), typeRecord);
332339

333-
/* CVSymbolSubsectionBuilder will add an associated S_UDT record to symbol table. */
340+
/* CVSymbolSubsectionBuilder will add associated S_UDT record to symbol table. */
334341
log(" finished class %s", typeRecord);
335342

336343
return typeRecord;

0 commit comments

Comments
 (0)