[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via llvm-commits]

rnk planned changes to this revision.
rnk marked 9 inline comments as done.
rnk added a comment.

In D60018#1448709 <https://reviews.llvm.org/D60018#1448709>, @zturner wrote:

> Nice, does this actually have any performance impact?  Since the arrays are smaller and  have better cache locality.


I haven't done any measurements, actually. I think it will affect the public stream, though, which maintains this vector:

  std::vector<CVSymbol> Records;

I got rid of the per-module vector of symbols, though, so I'm not sure how big it will be. I guess I'll just measure heap usage on clang.pdb before I commit.

In D60018#1448983 <https://reviews.llvm.org/D60018#1448983>, @aganea wrote:

> I like when removing code makes a class actually less complex :) (while keeping the same feature set)


I'm actually wondering if we should remove the size as well, since it's technically redundant with the RecordLen. Originally, while working on the dumper, we used `ArrayRef<uint8_t>` so that we had a way to make sure we didn't overrun our parent buffer, like an .debug$S subsection. But, I could imagine we might want to validate our inputs up front, and then pass around plain (or wrapped) record pointers in the linker for performance reasons. Hard to say.

Anyway, I agree, this change stands on its own as a good simplification. :)

---

It looks like there's a bunch of new CV code in LLDB that I have to go refactor, so I'll go do that, and then reupload.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:33
 
-  bool valid() const { return Type != static_cast<Kind>(0); }
+  bool valid() const { return kind() != static_cast<Kind>(0); }
 
----------------
aganea wrote:
> `return RecordData.size() >= sizeof(RecordPrefix)`
> 
> Do we expect an invalid (zero) `.RecordKind` here?
I don't think so.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:54
                                  CodeViewContainer Container) {
-    CVSymbol Result;
-    Result.Type = static_cast<SymbolKind>(Sym.Kind);
+    uint8_t PreBytes[sizeof(RecordPrefix)];
+    RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
----------------
aganea wrote:
> This seems like a recurrent pattern, why not add a constructor such as:
> `CVRecord(RecordPrefix* R) : RecordData((uint8_t*)R, R->RecordLen + 2) {}`
> 
> Which reduces all this code to:
> ```
> RecordPrefix R{ulittle16_t(2), ulittle16_t((uint16_t)Sym.Kind)};
> CVSymbol Result(&R);
> ```
> Which could be further improved if RecordPrefix had a `RecordPrefix(uint16_t, uint16_t)` constructor.
> 
In addition to those constructors, I was actually thinking maybe CVRecord should just point to a `RecordPrefix*` instead of using the ArrayRef wrapper, and then convert back to `uint8_t*` in `.data()` and `.content()`.

I tried your suggestion, but I screwed up the field order of RecordPrefix, so I think I'll leave it like this, since it's more self-documenting:
    RecordPrefix Prefix;
    Prefix.RecordLen = 2;
    Prefix.RecordKind = uint16_t(Sym.Kind);
    CVSymbol Result(&Prefix, sizeof(Prefix));

I want to pass a size along with the prefix pointer because the whole idea of using ArrayRef in the first place is to maintain the buffer size separately from the input data so we don't end up trusting it.


================
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);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018