[PATCH] D63671: [llvm-profdata] Avoid keeping reference to every files

[Wei Mi via llvm-commits]

On Thu, Jul 11, 2019 at 9:20 AM Paul Semel via Phabricator
<reviews at reviews.llvm.org> wrote:
>
> paulsemel added a comment.
>
> In D63671#1568997 <https://reviews.llvm.org/D63671#1568997>, @wmi wrote:
>
> > In D63671#1565126 <https://reviews.llvm.org/D63671#1565126>, @wmi wrote:
> >
> > > In D63671#1564933 <https://reviews.llvm.org/D63671#1564933>, @paulsemel wrote:
> > >
> > > > In D63671#1563092 <https://reviews.llvm.org/D63671#1563092>, @wmi wrote:
> > > >
> > > > > In D63671#1556584 <https://reviews.llvm.org/D63671#1556584>, @paulsemel wrote:
> > > > >
> > > > > > Add one test case.
> > > > >
> > > > >
> > > > > Thanks. If we remove the code to replace string reference from file data buffer to FunctionNames set for call targets, will the test fail?
> > > >
> > > >
> > > > No, because this change is kind of a NFC, which implies that I want to keep the same behaviors as previously.
> > > >  To actually have a test that fails if we remove this code, we would need to have a super heavy and time consuming test, which is, imo, not what we want for a test.
> > > >
> > > > Do we really need to have a tests that fails if we remove this code ?
> > >
> > >
> > > Is it possible to add an option to flush the file data buffer with 0 after the file is released? The option is only enabled for testing. With the option on, even with small test we can catch some difference in merge result if the patch is incorrect.
> >
> >
> > I realize I wasn't clear. I mean flush the data buffer after reading and merging for the file is done and just before the file is released.
>
>
> I am very sorry, but I don't really understand what you want... The files are mmap'd with only READ protection, so zeroing the data isn't really an option!
> Plus, we munmap the file when we destroy the object at the end of the loop, so accessing again the data would result in a segfault already... we cannot do much more here I think, but maybe I am missing smth ?
>

I assumed the input files are small since we want to have small
testcase. If the input files are small, the buffer won't be allocated
through mmap but through new (From getOpenFileImpl in
lib/Support/MemoryBuffer.cpp).

Thanks,
Wei.

>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D63671/new/
>
> https://reviews.llvm.org/D63671
>
>
>