- User Since
- Apr 27 2015, 11:17 AM (281 w, 5 d)
Add test options that exercise code, fix exposed bug.
I realized after intentionally suppressing the new method that there was no regression test that broke. It turns out that the lazy loading index is only emitted if there is a certain number of metadatas, which the tests that exercise this code apparently don't have. Therefore, I added an option to drop the threshold for one of the ThinLTO WPD tests. This exposed a bug: Because parseGlobalObjectAttachment now resolves forward references via the index, the index cursor has moved when it returns, and we weren't parsing all of the global decl attachment metadatas. Fixed by saving and restoring the current position around that call.
Thanks for doing this!
Fri, Sep 18
lgtm with a couple of minor suggestions
Thu, Sep 17
I had to revert due to 2 bot failures.
Implement test suggestion
Fix tmp file handling to work on all platforms
I realized I had to make a change, PTAL.
Wed, Sep 16
I pulled out the sanitizer_common handling for printing the stack depot into another patch. I also removed the internal_getcpu handling as I couldn't reproduce why I needed it in the first place. Calling sched_getcpu seems to work fine without it.
Remove sanitizer common handling from this patch
Tue, Sep 15
LGTM, just needs a comment update as noted below and also an update to the patch title.
Remove a few extraneous files from commit. I will be splitting out the
sanitizer_common changes into another patch shortly, and will update the
patch with a note afterwards when this is ready to review again.
Mon, Sep 14
Rename from heapprof to memprof.
Sun, Sep 13
Sat, Sep 12
There are a number of single statement if and for bodies in the patch that have braces but should not per llvm coding style.
Please mention in the summary somewhere that this is only enabled for lld right now.
Fri, Sep 11
Can you rebase this on top of D86913 so that it doesn't include those changes?
Manually fixed the formatting issues causing clang-format to get confused
The change itself looks good to me, but I don't understand this comment from the summary:
Thu, Sep 10
Fix incorrect clang-format
Remove offset field and use from_memalign
Address most of the comments
The next patch update should address all of the comments except:
Rebase and update flag
LGTM with comment updates as suggested below.
Wed, Sep 9
Tue, Sep 8
Responses to some of @MaskRay's comments below (if no explicit response then ack).
Does this rely on the following check in EmbedBitcodeInModule returning true?
Why not just make this one an NFC change to remove the unused Conf parameter. Then pass in CmdLineOpts in the patch where its use is added?
Fri, Sep 4
Thanks for taking a look. A couple questions/comments below. I'll address the other ones.
Thu, Sep 3
Wed, Sep 2
I need to think through this some more, but a couple questions:
AFAICT this looks ok, but adding @dblaikie as he is more familiar with the DI metadata. Couple minor comments below.
Tue, Sep 1
Thanks for your comments, @etiotto. I'll address them in the next day or so. Alternate suggestion below for flag.
I'm ok with regular LTO implementation going in first, but it would be great to have a ThinLTO follow on implementation. Happy to provide pointers on that when you are ready.
Thu, Aug 27
Looks like this got fixed in the AddressSanitizer code after I cloned it over...
Wed, Aug 26
Remove unnecessary check
Tue, Aug 25
I think I've addressed all the comments.
@pcc ping. I'd like to get something in for LLVM 11 if at all possible. How about for now I make the last sentence something like "This is useful in situations where it is not safe to specify`-fvisibility=hidden` at compile time, for example when bitcode objects will be consumed by different LTO links that don't all have whole program visibility." ?