Skip to content

Commit 9aa5b5b

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

File tree

2 files changed

+67
-72
lines changed

2 files changed

+67
-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: 51 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,56 @@ 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+
/*
173+
* The last FieldList must refer back to the one before it, and must contain the first
174+
* fields in the class.
175+
*/
176+
177+
CVTypeRecord.CVFieldListRecord currentFieldList = new CVTypeRecord.CVFieldListRecord();
178+
Deque<CVTypeRecord.CVFieldListRecord> fl = new LinkedList<>();
179+
fl.add(currentFieldList);
180+
181+
/* Build all Field List records in field order (FIFO). */
182+
for (CVTypeRecord.FieldRecord fieldRecord : fields) {
183+
if (currentFieldList.getEstimatedSize() + CVUtil.align4(fieldRecord.computeSize()) >= CV_TYPE_RECORD_MAX_SIZE) {
184+
currentFieldList = new CVTypeRecord.CVFieldListRecord();
185+
fl.add(currentFieldList);
186+
}
187+
currentFieldList.add(fieldRecord);
188+
}
189+
190+
/*
191+
* Emit all Field List records in reverse order (LIFO), adding Index records to all but
192+
* the first emitted.
193+
*/
194+
CVTypeRecord.CVFieldListRecord fieldListRecord = null;
180195
int idx = 0;
181-
while (!lists.isEmpty()) {
182-
CVTypeRecord.CVFieldListRecord fieldList = lists.removeLast();
183-
currentFieldList = builder.addTypeRecord(fieldList);
196+
while (!fl.isEmpty()) {
197+
fieldListRecord = fl.removeLast();
198+
fieldListRecord = builder.addTypeRecord(fieldListRecord);
184199
/* For all fieldlist but the first, link to the previous record. */
185200
if (idx != 0) {
186-
fieldList.add(new CVTypeRecord.CVIndexRecord(idx));
201+
fieldListRecord.add(new CVTypeRecord.CVIndexRecord(idx));
187202
}
188-
idx = currentFieldList.getSequenceNumber();
203+
idx = fieldListRecord.getSequenceNumber();
189204
}
190-
return currentFieldList;
205+
return fieldListRecord;
191206
}
192207
}
193208

@@ -212,26 +227,22 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
212227

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

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

223234
if (superTypeIndex != 0) {
224235
CVTypeRecord.CVBaseMemberRecord btype = new CVTypeRecord.CVBaseMemberRecord(MPROP_PUBLIC, superTypeIndex, 0);
225236
log("basetype %s", btype);
226-
fieldListContext.addRecordToFieldList(btype);
237+
fieldListBuilder.addField(btype);
227238
}
228239

229240
/* Only define manifested fields. */
230241
typeEntry.fields().filter(CVTypeSectionBuilder::isManifestedField).forEach(f -> {
231242
log("field %s attr=(%s) offset=%d size=%d valuetype=%s", f.fieldName(), f.getModifiersString(), f.getOffset(), f.getSize(), f.getValueType().getTypeName());
232243
CVTypeRecord.FieldRecord fieldRecord = buildField(f);
233244
log("field %s", fieldRecord);
234-
fieldListContext.addRecordToFieldList(fieldRecord);
245+
fieldListBuilder.addField(fieldRecord);
235246
});
236247

237248
if (typeEntry.isArray()) {
@@ -250,7 +261,7 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
250261
/* Build a field for the 0 length array. */
251262
CVTypeRecord.CVMemberRecord dm = new CVTypeRecord.CVMemberRecord(MPROP_PUBLIC, array0record.getSequenceNumber(), typeEntry.getSize(), "data");
252263
log("field %s", dm);
253-
fieldListContext.addRecordToFieldList(dm);
264+
fieldListBuilder.addField(dm);
254265
}
255266

256267
/*
@@ -292,25 +303,25 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
292303

293304
/* LF_METHOD record */
294305
CVTypeRecord.CVOverloadedMethodRecord methodRecord = new CVTypeRecord.CVOverloadedMethodRecord((short) nmlist.count(), nmlist.getSequenceNumber(), mname);
295-
fieldListContext.addRecordToFieldList(methodRecord);
306+
fieldListBuilder.addField(methodRecord);
296307
});
297308

298309
methods.stream().filter(methodEntry -> !overloaded.contains(methodEntry.methodName())).forEach(m -> {
299310
log("`unique method %s %s %s(...)", m.fieldName(), m.methodName(), m.getModifiersString(), m.getValueType().getTypeName(), m.methodName());
300311
CVTypeRecord.CVOneMethodRecord method = buildMethod((ClassEntry) typeEntry, m);
301312
log(" unique method %s", method);
302-
fieldListContext.addRecordToFieldList(method);
313+
fieldListBuilder.addField(method);
303314
});
304315
}
305316
/* Build fieldlist record from manifested fields. */
306-
CVTypeRecord.CVFieldListRecord newfieldListRecord = fieldListContext.addTypeRecords(this);
317+
CVTypeRecord.CVFieldListRecord newfieldListRecord = fieldListBuilder.buildFieldListRecords(this);
307318
int fieldListIdx = newfieldListRecord.getSequenceNumber();
308-
int fieldListCount = newfieldListRecord.count();
319+
int fieldCount = fieldListBuilder.getFieldCount();
309320
log("finished building fieldlist %s", newfieldListRecord);
310321

311322
/* Build final class record. */
312323
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);
324+
CVTypeRecord typeRecord = new CVTypeRecord.CVClassRecord(LF_CLASS, (short) fieldCount, attrs, fieldListIdx, 0, 0, typeEntry.getSize(), typeEntry.getTypeName(), null);
314325
typeRecord = addTypeRecord(typeRecord);
315326

316327
if (typeEntry.isClass()) {
@@ -330,7 +341,7 @@ private CVTypeRecord buildStructureTypeEntry(final StructureTypeEntry typeEntry)
330341

331342
types.typeNameMap.put(typeEntry.getTypeName(), typeRecord);
332343

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

336347
return typeRecord;

0 commit comments

Comments
 (0)