[PATCH] D41297: [ThinLTO] Implement summary visualizer

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 12 08:58:20 PST 2018


mehdi_amini added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:77
+  /// Summary string representation. Valid for combined summaries
+  StringRef Name;
+
----------------
evgeny777 wrote:
> tejohnson wrote:
> > I have a concern about keeping this string here. Who owns the string? I guess it is the string table created during bitcode reading. Do those even stay live through reading all input bitcode files and building the combined index? If not, saving a reference here is a problem. If so, then we currently have overhead that isn't typically necessary (we don't operate on IR or the names during the thin link).
> > 
> > I guess you added this to avoid needing the script to annotate these names instead of using the guids. Assuming we need to do something to keep these names live, I wonder if we can keep these names around somewhere under a debugging option instead (and emit the GUID by default). 
> >  Who owns the string?
> 
> My understanding is that linker owns this string because it loads bitcode file to memory and keeps it there during entire link phase. This StringRef points to strtab of the module, which is, to my understanding, is a region within MemoryBuffer owned by lld. Not so sure about gold.
> 
> I've found a problem though with VST_CODE_ENTRY/VST_CODE_FINENTRY. When they're being parsed their names are created on stack. This should be fixed.
> 
> > I wonder if we can keep these names around somewhere under a debugging option instead (and emit the GUID by default).
> 
> Sounds reasonable, if there is no other way. I'll take a look on code reading/writing VST entries 
> 
> My understanding is that linker owns this string because it loads bitcode file to memory and keeps it there during entire link phase

At which point of the API is this guarantee? What about other tools than the linker that would manipulate a combined summary?


https://reviews.llvm.org/D41297





More information about the llvm-commits mailing list