[PATCH] D26253: [CodeView] Hook CodeViewRecordIO up to the type serialization path.

[Zachary Turner via llvm-commits]

zturner added inline comments.


================
Comment at: include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h:26
 
+#include <stdint.h>
+
----------------
inglorion wrote:
> Out of curiosity, why move this include? And, while no the subject, why not use cstdint?
I tend to include files in order from most dependencies to fewest dependencies.  This way you don't have hidden transitive dependencies picked up through previous include, helping to enforce include-what-you-use.  C++ headers can depend on C headers, but not the other way around, therefore I tend to prefer including C++ system headers before C system headers.


================
Comment at: lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:148
 Error CodeViewRecordIO::mapGuid(StringRef &Guid) {
+  if (16 > maxFieldLength())
+    return make_error<CodeViewError>(cv_error_code::insufficient_buffer);
----------------
inglorion wrote:
> Do we need to perform this check when reading, too? If not, could you move it after the assert(Guid.size() ...)?
> 
> Also, I would make a symbolic constant, particularly since it's repeated a number of times. Believe it or not, it wasn't immediately clear to me where the 16 came from (yes, I'm having my caffeine now).
I theory we don't need to use these checks when reading, but it would be kind of interesting if one ever fired.  Currently we believe they should never fire, but if it were to fire on a non-Clang-generated PDB, then it might mean we don't know as much as we thought we did about the format, because it means we have a record larger than the size we thought was the maximum.  On the other hand, if it fired on a clang-generated PDB, then it would indicate we did something wrong (which in theory should have been caught during writing, but who knows).

I think it will be useful at some point or another because once we get libfuzzer stood up on Windows, we may want to start fuzzing PDBs, and at that point all bets are off.


https://reviews.llvm.org/D26253