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

[Rui Ueyama via llvm-commits]

How can you guarantee that `finalize' to be an atomic operation? Even if we
postpone `addStream' calls, we eventually have to call `addStream' for each
stream in `finalize', and any one of them can fail, and if it happens in
the middle of series of operations, we have orphan streams because we have
no way to roll them back.

On Mon, Oct 10, 2016 at 3:35 PM, Zachary Turner <zturner at google.com> wrote:

> 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/046f90a6/attachment.html>