<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 19, 2014 at 5:29 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> This patch addresses the first stage of PR17891 by folding constant<br>
> arguments together into a single MDString.  Integers are stringified and<br>
> a `\0` character is used as a separator.<br>
<br>
</span>I've attached updated LLVM and clang patches, rebased this afternoon<br>
with all tests in `check-all` passing.  I believe it addresses David's<br>
comments as well.<br>
<br>
There are an insane number of changes in the testcases, but changing the<br>
schema of the dwarf tags makes that inevitable -- almost every line of<br>
metadata had to change.  I look forward to "stage 2" of PR17891, which<br>
can be more incremental.<br>
<br>
Let me know if the updated patches look good.<br></blockquote><div><br></div><div>General idea looks good - might be easier to do the micro-review stuff in a Phab review (some things I saw: <br>* DIHeaderIterator could probably just be a generic StringIterator (could be templated on a delimiter or just hardcoded in the form of a NullSeparatedStringIterator?)? - again, using the DI prefix for something not in the DI* hierarchy is something I twitch about a bit... though DIRef, etc, are precedent in favor of your naming there<br>* getHeaderFields could probably return a range rather than having to build an entire vector<br>* getHeaderField(unsigned) could probably use std::advance<br>* DIHeader would go well with the StringIterator above, mabye NullSeparatedStringBuilder? It sort of feels like a builder-y thing... <br>* Why the "Twinish" perfect forwarding in DIHeader::concat? Rather than just taking a Twine?</div><div>* I guess the builder could be just a variadic function one day... *sigh*<br>)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Two caveats that came to mind while writing this:<br>
<br>
  - I haven't updated `docs/SourceLevelDebugging.rst`.  I'll send a<br>
    patch for that Monday-ish.<br>
<br>
  - I haven't bumped the "Debug Info Version".  Should I?<br></blockquote><div><br></div><div>You'll have to check with Adrian about that - I think the notion was to do that once per schema-changing LLVM open source release. Otherwise you'd have to change it on every schema change and that involves changing every debug info metadata test file :/ (& I'd really like us not to churn it /that/ much if we can help it)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Any size measurements/test results (does this bootstrap?) on this?<br>
<br>
</span>Before committing, I'll use a release+asserts clang to build:<br>
<br>
  - opt with `-g -flto`<br>
  - clang with `-g`<br>
<br>
With the LTO link of opt, I'll confirm that there's a drop in memory<br>
usage due to fewer MDNodeOperands and look out for time regressions.<br>
<br>
</blockquote></div><br></div></div>