aganea added inline comments.

================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
 template <typename Kind> class CVRecord {
----------------
To add to what you said in a comment above, do you think that if we could add 
`assert(Data.size() == ((RecordPrefix *)RecordData.data())->RecordPrefix + 2)` 
at relevant places below; and then after a while we could simply switch to 
`RecordPrefix *`, once issues are ironed out?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:56
+    RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
+    Prefix->RecordLen = 0;
+    Prefix->RecordKind = uint16_t(Sym.Kind);
----------------
rnk wrote:
> aganea wrote:
> > Should be 2.
> I could say 2, but this is the seralizer, so really it just gets overwritten. 
> Do you think I should leave it uninitialized, 2, -1, 0?
I'd say "2" because you want as much as possible to keep data coherent in time. 
Regardless of what comes after. I think the code should be correct before being 
optimal. If someone looks for `RecordPrefix` and then tries to understand how 
it works, it'd be nice if the code displays the same, correct, behavior 
everywhere.

But then, you can argue that `RecordPrefix.RecordLen` is simply a constrait, 
tied to the amount data that comes after. And maybe the calculation logic for 
that `.RecordLen` field should be hidden inside the `RecordPrefix` class. In 
that case, to avoid screwing the init order, ideally you could simply say:
```
    RecordPrefix Prefix{uint16_t(Sym.Kind)};
    CVSymbol Result(&Prefix);
```
...and let `RecordPrefix` (or `CVSymbol`) handle sizing?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h:125
+    Pre.RecordKind = uint16_t(TypeLeafKind::LF_FIELDLIST);
+    CVType FieldList(&Pre, sizeof(Pre));
     consumeError(Mapping.Mapping.visitTypeEnd(FieldList));
----------------
Applying what I said above would give:
```
    RecordPrefix Pre{uint16_t(TypeLeafKind::LF_FIELDLIST)};
    CVType FieldList(&Pre);
```


================
Comment at: llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp:37
 
+  CVType CVT(ArrayRef<uint8_t>(ScratchBuffer.data(), sizeof(RecordPrefix)));
   cantFail(Mapping.visitTypeBegin(CVT));
----------------
Here it is the same thing: `writeRecordPrefix()` writes a `.RecordLen = 0`, but 
it could very well write 2 to be coherent, then you would be able to //trust// 
the `RecordPrefix *`.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:40
+      static CVSymbol Tombstone(
+          ArrayRef<uint8_t>(DenseMapInfo<uint8_t *>::getTombstoneKey(), 1));
       return Tombstone;
----------------
I suppose you've chosen 1 because that's a invalid record size? Comment maybe?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60018/new/

https://reviews.llvm.org/D60018



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to