- User Since
- Apr 27 2015, 11:17 AM (283 w, 2 d)
Ping - could someone take a quick look at the update and approve the recommit?
Sat, Sep 26
Fri, Sep 25
Fix bug and add test case.
I have a fix for the issue that required a revert. We shouldn't be using the lazy loading IndexCursor to load the global decl attachment metadata. That cursor is initialized with the necessary abbrev ids from the module level metadata block at the end of the lazy loading index setup, so that you can use those abbrev ids when jumping into random locations, as noted in the comment above its declaration:
I'm not the right person to review this change, as we don't use the old LTO API at all. But I'll just add a quick note that there is a way to pass this through the new LTO API (lto::Config::CGFileType), and options to set it from lld/gold-plugin/llvm-lto2.
Thu, Sep 24
@vitalybuka Ping - let me know if you want me to split it up further before more review. I'm not sure the best way to split it up, since a lot of this is the basic infrastructure that works together. The main thing I can think of is to split it up onto 2 pieces: 1) with the infrastructure that just allows the heap profiler to intercept and handle the memory allocations/deallocations/intrinsics (which was largely cloned from the asan code); and 2) the part on top of that which does the actual heap tracking (the MemInfoBlock and MemInfoBlockCache stuff in memprof_allocator.cpp). If I split it I would keep most of the the chunk header part in the first patch, since it affects the memory allocations/alignment, but just not fill in the fields related to the heap tracking.
Tue, Sep 22
lgtm with comment typo fix
Is it expected that the user will want to specify this only at link time and not during compile time? If it is normally specified in the compile step, you should consider adding it to the IR in some fashion (e.g. function attributes). The advantage is that the user doesn't need to pass different options for the LTO and non-LTO cases.
LGTM with comment suggestion.
Mon, Sep 21
Sat, Sep 19
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.
Aug 27 2020
Looks like this got fixed in the AddressSanitizer code after I cloned it over...