[PATCH] D64428: Teach `llvm-pdbutil pretty -native` about `-injected-sources`

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 16 10:38:52 PDT 2019


rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, up to you if you want to wait for @zturner. I like the new error handling approach. :)



================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumInjectedSources.cpp:50
+    if (!Name) {
+      consumeError(Name.takeError());
+      return "(invalid file name ref)";
----------------
thakis wrote:
> thakis wrote:
> > zturner wrote:
> > > rnk wrote:
> > > > Hm. I guess perhaps we should've allowed error propagation out of these APIs. I guess that's not fixable without an interface change.
> > > The thinking at the time was that anything that might generate an error should happen during load, not on lazy evaluation of the properties.  That's why most of the classes have a `initialize` or `reload()` static method which returns an `Expected<T>`, so that all validation can be done up front.  I'm open to the possibility that that wasn't the best decision though, but it kept the code simple and handled most of the cases we would expect to see in practice.
> > Thanks for chiming in! That makes sense to me.
> > 
> > I changed the code to verify that all referenced strings exist in `reload()` and removed the error handling here. I didn't (yet) do this for getCode() since that'd duplicate quite a bit of code. From your comment, it kind of sounds like the intended design is that InjectedSourceStream should grab _all_ data from disk and store it in memory, and the NativeEnumInjectedSources should refer to that. Is that accurate? If so, InjectedSourceStream should probably have a
> > 
> > ```
> >   struct InjectedSource {
> >     uint32_t CRC32;
> >     StringRef Name;
> >     StringRef ObjName;
> >     StringRef VName;
> >     PDB_SourceCompression Compression;
> >     llvm::MemoryBuffer Contents;
> >   };
> > ```
> > 
> > and store a name->InjectedSource map, and check that the CRC32 matches the CRC32 of Contents at load time too. Does that sound right?
> > 
> > If so, is there an existing method to load the contents of a stream into a MemoryBuffer somewhere?
> Looking at this some more: For just natvis files, loading everything into memory seems fine, but the docs for IDiaInjectedSource kind of suggest that it's possible to store the source code of all cc files compiled to obj files in the pdb, and loading that eagerly would be quite a bit of data. OTOH, it's only loaded eagerly if PDBFile::getInjectedSourceStream() is called, and the only caller of it currently walks all entries anyhow.
> If so, is there an existing method to load the contents of a stream into a MemoryBuffer somewhere?

I don't think so, we moved data into and out of the stream APIs with ArrayRef<uint8_t>, typically stored in BumpPtrAllocators.


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

https://reviews.llvm.org/D64428





More information about the llvm-commits mailing list