Page MenuHomePhabricator


Authored by ruiu on Jun 14 2016, 4:29 PM.

Diff Detail


Event Timeline

ruiu updated this revision to Diff 60779.Jun 14 2016, 4:29 PM
ruiu retitled this revision from to [pdbdump] Verify LF_{CLASS,ENUM,INTERFACE,STRUCTURE,UNION} records..
ruiu updated this object.
ruiu added a reviewer: zturner.
ruiu added a subscriber: llvm-commits.
ruiu added a comment.Jun 14 2016, 4:32 PM

I tried to use the visitor, but it didn't fit well this case. If I had used the visitor,
we would have had much more code here. So I used a simple template function
instead of the visitor class.

zturner added inline comments.Jun 14 2016, 8:10 PM
78 ↗(On Diff #60779)

I'm not sure if corrupt_file is the best error code to return here, because you could construct one of these out of thin air instead of coming from a file. Maybe we should have a new code called invalid_type_record.

81 ↗(On Diff #60779)

Is the static_cast necessary? The enum should have overloaded & operator so you can do the operation directly.

81 ↗(On Diff #60779)

What if T doesn't have a getOptions method? Will this be used for other values of T in the future? If so I think we should use the visitor instead, since it isolates the handling of each type in a separate function

82 ↗(On Diff #60779)

Are these two static_casts necessary? The enum should have overloaded & operator so you can do the operation directly.

90 ↗(On Diff #60779)

Is it always hashStringV1? I would think it depends on the version of the Tpi stream. I don't think we have a way to test this as I don't know how to generate a PDB file with a newer tpi stream version. Does the reference implementation hardcode a call to this function or does it go through a function pointer which might be hashStringV1 or hashStringV2?

118 ↗(On Diff #60779)

If there were less code required to use the visitor, would that be preferable? I have an idea in mind to drastically reduce the amount of code needed to use it which I've been putting off. If you think we're going to add more values to this switch statement in subsequent patches, it's probably worth improving the visitor, but if you think this will be all, then maybe it's not necessary.

ruiu added inline comments.Jun 15 2016, 11:04 AM
78 ↗(On Diff #60779)


81 ↗(On Diff #60779)

static_cast is necessary since ClassOptions is a enum class.

If T doesn't have a getOptions method, it fails to compile, but it should be fine because the classes we have here are only ones that we need to handle (unless Microsoft extends the PDB file format in future.)

90 ↗(On Diff #60779)

Yes, the reference hard-codes hashStringV1.

118 ↗(On Diff #60779)

I believe this will be all. The problems with using the visitor here are, first, it needs more code to do the same thing, and second, we are not only reading type records but also hash records. We are reading parallel arrays. That's doable with the visitor, but it's more complicated than usual.

ruiu updated this revision to Diff 60865.Jun 15 2016, 11:05 AM
  • Address review comments.
zturner accepted this revision.Jun 15 2016, 11:22 AM
zturner edited edge metadata.

looks good. After posting this review I found out we already have cv_error_code::corrupt_record. So probably we should use that as follows:

return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
This revision is now accepted and ready to land.Jun 15 2016, 11:22 AM
This revision was automatically updated to reflect the committed changes.