[llvm-commits] Thread Sanitizer seems to have the dual issue

[Kostya Serebryany]

I see the problem now.

With your patch we have:
  - asan calls doInitialization and creates Function objects (by calling
M.getOrInsertFunction).
  - some other pass kicks in and removes those Functions as unused
  - asan calls runOnFunction and expects the Function objects to be alive,
but they are not.

The fix is simple: move the M.getOrInsertFunction calls into runOnFunction.
Is that what we want?
This will at least be a compile performance degradation (though probably
not significant for the majority of cases).

Evgeniy, you may also need to do this change in msan pass.

--kcc

On Wed, Nov 28, 2012 at 10:20 PM, Pedro Artigas <partigas at apple.com> wrote:

> Hello All,
>
> First, thanks for addressing the issue in AddressSanitizer, now make check
> with my patch passes the ASAN test, which means the patch for it worked as
> desired.
>
> Unfortunately the build/tools/clang test shows issues with my patch on the
> TSAN test (there is also an ASAN issue that I did not notice before, but
> seems to be orthogonal from the doFinalization one that got fixed), I am
> attaching the patch and the output of make test in the clang directory.
>
>
>
>
> I believe this is an issue due to the re-ordering of the call
> doInitialization because when I hacked the code to preserve the original
> ordering all tests passed, the current ordering is:
>
> ModulePasses doInitialization
> Module Passes run
> Function Passes doInitialization
> Function Passes run
> Function Passes doFinalization
> ModulePasses doFinalization
>
> The new ordering with my patch is:
>
> ModulePasses doInitialization
> Function Passes doInitialization
> Module Passes run
> Function Passes run
> Function Passes doFinalization
> ModulePasses doFinalization
>
> Note that the initialization/run/finalization are now clustered as the
> proposal mentioned they should be. The issue seems to be that TSAN prefers
> the doInitialization of the function pass to happen after the prior module
> pass runs. Unfortunately that ordering is not compatible with the model.
> Again if I hack in that order the tests pass but that is supposed to not be
> a legal order.
>
> Thanks for any info,
>
> Pedro
>
>
> On Nov 28, 2012, at 1:41 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> > On Tue, Nov 27, 2012 at 8:50 PM, Kostya Serebryany <kcc at google.com>
> wrote:
> >> Hi Pedro,
> >>
> >> I managed to understand the problem with doFinalization, but I still
> don't
> >> see the problem with doInitialization.
> >> Could you please describe what's wrong in more detail?
> >
> > I don't either. I've read both pass's doInitialization routines (both
> > ASan and TSan) and I'm quite convinced they work with the intended
> > end-goal of:
> >
> > For each module compiled:
> > 1) all passes doInitialization routines are called exactly once with
> > that module passed into it.
> > 2) the pass manager's runOn* sequence occurs, running passes (perhaps
> > multiple times based on the pass manager) across the module.
> > 3) all passes doFinalization routines are called exactly once with
> > that module passed into it.
> >
> > ThreadSanitizer and AddressSanitizer both use doInitialization to set
> > up boring per-module state necessary for the passes to operate:
> > 1) They register a function in the set of global ctors
> > 2) They add declarations for runtime library functions
> > 3) The create some global variables
> >
> > These things seem completely fine and reasonable to do in
> > doInitialization, so I think we'll need some more details to
> > understand what is actually going wrong here.
> >
> > Can you post a patch? Can you post the error messages you're seeing?
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121129/0620c9a3/attachment.html>