[PATCH] D25356: Define PDBFileBuilder::addStream to add stream.

[Zachary Turner via llvm-commits]

Currently we're not checking anything before calling `addStream`.  It's the
first line of `addDbgStream`.  So even if the function would return an
error because it already exists, the extra stream would already have been
created.  I suppose if you add a check for that it would be fine.  But
conceptually `finalize` is the place where modification to the underlying
MSF is supposed to happen.  At the very least though we should be checking
in `addStream` and returning an error immediately if someone has already
added that stream (although that would prevent you from adding more data to
the stream later on, for example).

On Mon, Oct 10, 2016 at 3:12 PM Rui Ueyama <ruiu at google.com> wrote:

> ruiu added inline comments.
>
>
> ================
> Comment at: lib/DebugInfo/PDB/Raw/DbiStreamBuilder.cpp:49
> +                                     ArrayRef<uint8_t> Data) {
> +  auto ExpectedIndex = Msf.addStream(Data.size());
> +  if (!ExpectedIndex)
> ----------------
> zturner wrote:
> > I think we shouldn't call `addStream` here.  Suppose someone called
> `addDbgStream` more than once for the same value of `Type`.  Then you would
> end up with unreferenced streams in the file.  Maybe instead we can just
> store the `ArrayRef`, overwriting any previous value of the `ArrayRef` that
> was there before.  Then in `finalizeMsfLayout` we can add all the streams.
> >
> > This way everything is semantically "atomic".  No modification to the
> underlying file happens until you call `finalize()`, so you can do whatever
> you want until then without risk of messing anything up.
> How about this? I think this is atomic because we check everything that
> can fail before calling addStream in this function.
>
>
> https://reviews.llvm.org/D25356
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161010/7d21e7b5/attachment.html>