<div dir="ltr">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.</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 10, 2016 at 3:35 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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).</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 10, 2016 at 3:12 PM Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ruiu added inline comments.<br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
================<br class="m_6750775339895170796gmail_msg">
Comment at: lib/DebugInfo/PDB/Raw/<wbr>DbiStreamBuilder.cpp:49<br class="m_6750775339895170796gmail_msg">
+                                     ArrayRef<uint8_t> Data) {<br class="m_6750775339895170796gmail_msg">
+  auto ExpectedIndex = Msf.addStream(Data.size());<br class="m_6750775339895170796gmail_msg">
+  if (!ExpectedIndex)<br class="m_6750775339895170796gmail_msg">
----------------<br class="m_6750775339895170796gmail_msg">
zturner wrote:<br class="m_6750775339895170796gmail_msg">
> 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.<br class="m_6750775339895170796gmail_msg">
><br class="m_6750775339895170796gmail_msg">
> 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.<br class="m_6750775339895170796gmail_msg">
How about this? I think this is atomic because we check everything that can fail before calling addStream in this function.<br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
<a href="https://reviews.llvm.org/D25356" rel="noreferrer" class="m_6750775339895170796gmail_msg" target="_blank">https://reviews.llvm.org/<wbr>D25356</a><br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
<br class="m_6750775339895170796gmail_msg">
</blockquote></div>
</div></div></blockquote></div><br></div>