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

Thu Jul 11 09:20:53 PDT 2019

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 ?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63671/new/

https://reviews.llvm.org/D63671