This is an archive of the discontinued LLVM Phabricator instance.

[PDB] Move PDB functions to a separate file.
ClosedPublic

Authored by ruiu on Jun 8 2016, 9:59 AM.

Details

Summary

We are going to use the hash functions from TPI streams.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu updated this revision to Diff 60057.Jun 8 2016, 9:59 AM
ruiu retitled this revision from to [PDB] Move PDB functions to a separate file..
ruiu updated this object.
ruiu added a reviewer: zturner.
ruiu added a subscriber: llvm-commits.
zturner accepted this revision.Jun 8 2016, 10:19 AM
zturner edited edge metadata.

There is also SigForPbCb in Microsoft headers. I haven't ported this one over yet, and I don't know the details about when each particular function is used.

If possible, could you add a comment over each function in Hash.h that briefly documents the situations in which that particular is used? i.e. what types of records, Tpi Stream version, etc. We can improve the comments as we learn more, but it would be nice for someone to look at the header file and have a general idea of why there are so many different hash functions.

This revision is now accepted and ready to land.Jun 8 2016, 10:19 AM
ruiu updated this revision to Diff 60065.Jun 8 2016, 10:25 AM
ruiu edited edge metadata.

Added brief comments about where these hash functions are used.
But I think it is better to describe how they are used where they
are used (instead of where the hash functions are defined,) so
it's not that long comments.

majnemer added inline comments.
lib/DebugInfo/PDB/Raw/Hash.cpp
21 ↗(On Diff #60057)

size_t ?

30 ↗(On Diff #60057)

Isn't this just Size % 4?

ruiu added inline comments.Jun 8 2016, 4:00 PM
lib/DebugInfo/PDB/Raw/Hash.cpp
22 ↗(On Diff #60065)

It shouldn't be that large as Str always points to a string in PDB (in which all streams are at most 2^32 bytes.)

31 ↗(On Diff #60065)

Done.

ruiu updated this revision to Diff 60112.Jun 8 2016, 4:00 PM
  • address review comments.
This revision was automatically updated to reflect the committed changes.

Hi Rui.

This patch breaks the LLVM build on Windows. For example:
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/12912

Seems the new Hash.cpp and Hash.h need <cstdint>.

Regards,
Christof

aaboud added a subscriber: aaboud.Jun 9 2016, 9:27 AM
aaboud added inline comments.
llvm/trunk/include/llvm/DebugInfo/PDB/Raw/Hash.h
14

This patch broke the build, please add this line to fix it:
#include <stdint.h>

ruiu added a subscriber: ruiu.Jun 9 2016, 11:03 AM

I'm sorry about the breakage. Hans fixed it for me in r272269.