[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D79698#2037425 , @aeubanks wrote:

> In D79698#2037305 , @leonardchan 
> wrote:
>
> > Just to followup on this: We believe the root cause of the error we're 
> > running into is that some sancov guards in one comdat are being referenced 
> > by symbols in another comdat, so due to linking order, the other comdat in 
> > question is referencing a sancov guard that was in a discarded comdat 
> > group. I posted full details of this on llvm-dev 
> > . We believe 
> > we're running into this error now because with this patch, inlining occurs 
> > after the sanitizers are run.
> >
> > Based on some discussion in 
> > http://lists.llvm.org/pipermail/llvm-dev/2020-May/141558.html, it seems 
> > appropriate that the solution for this is to just run sanitizers after 
> > inlining, so we would avoid this error that's brought about by inlining 
> > after sanitizer runs. Since this has been breaking us for a few days, and 
> > unless other folks don't mind, I'm thinking it might be appropriate to 
> > temporarily revert this until there's a fix for either
>
>
> Maybe the fix is to use `registerScalarOptimizerLateEPCallback()` instead of 
> `registerPipelineStartEPCallback()`. But I'm not sure how to test that, could 
> you try that and lmk if that fixes the issue?


Thanks. Will try this out later.

>> (1) running sanitizers after inlining or
>>  (2) changing which comdat groups the sancov guards get placed in (such that 
>> guards in one group aren't referenced by functions defined in another group)
>> 
>> I'm not sure how long it would take to implement either solution, so I think 
>> temporarily reverting might be ok here.
> 
> Sure, feel free to revert.

Reverted in e9802aa4221ba3857041c2328639ce2aac0ace67 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79698



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D79698#2037305 , @leonardchan wrote:

> Just to followup on this: We believe the root cause of the error we're 
> running into is that some sancov guards in one comdat are being referenced by 
> symbols in another comdat, so due to linking order, the other comdat in 
> question is referencing a sancov guard that was in a discarded comdat group. 
> I posted full details of this on llvm-dev 
> . We believe 
> we're running into this error now because with this patch, inlining occurs 
> after the sanitizers are run.
>
> Based on some discussion in 
> http://lists.llvm.org/pipermail/llvm-dev/2020-May/141558.html, it seems 
> appropriate that the solution for this is to just run sanitizers after 
> inlining, so we would avoid this error that's brought about by inlining after 
> sanitizer runs. Since this has been breaking us for a few days, and unless 
> other folks don't mind, I'm thinking it might be appropriate to temporarily 
> revert this until there's a fix for either


Maybe the fix is to use `registerScalarOptimizerLateEPCallback()` instead of 
`registerPipelineStartEPCallback()`. But I'm not sure how to test that, could 
you try that and lmk if that fixes the issue?

> (1) running sanitizers after inlining or
>  (2) changing which comdat groups the sancov guards get placed in (such that 
> guards in one group aren't referenced by functions defined in another group)
> 
> I'm not sure how long it would take to implement either solution, so I think 
> temporarily reverting might be ok here.

Sure, feel free to revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79698



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Just to followup on this: We believe the root cause of the error we're running 
into is that some sancov guards in one comdat are being referenced by symbols 
in another comdat, so due to linking order, the other comdat in question is 
referencing a sancov guard that was in a discarded comdat group. I posted full 
details of this on llvm-dev 
. We believe 
we're running into this error now because with this patch, inlining occurs 
after the sanitizers are run.

Based on some discussion in 
http://lists.llvm.org/pipermail/llvm-dev/2020-May/141558.html, it seems 
appropriate that the solution for this is to just run sanitizers after 
inlining, so we would avoid this error that's brought about by inlining after 
sanitizer runs. Since this has been breaking us for a few days, and unless 
other folks don't mind, I'm thinking it might be appropriate to temporarily 
revert this until there's a fix for either

(1) running sanitizers after inlining or
(2) changing which comdat groups the sancov guards get placed in (such that 
guards in one group aren't referenced by functions defined in another group)

I'm not sure how long it would take to implement either solution, so I think 
temporarily reverting might be ok here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79698



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

This patch seems to lead to the undefined symbol error we're seeing when 
building fuchsia:

  ld.lld: error: relocation refers to a discarded section: __sancov_guards
  >>> defined in 
user-x64-sancov.shlib/obj/third_party/ulib/scudo/scudo.string_utils.cc.o
  >>> referenced by vector.h:58 
(../../zircon/third_party/ulib/scudo/vector.h:58)
  >>>   
user-x64-sancov.shlib/obj/third_party/ulib/scudo/scudo.string_utils.cc.o:(scudo::ScopedString::append(char
 const*, __va_list_tag*))
  >>> referenced by vector.h:58 
(../../zircon/third_party/ulib/scudo/vector.h:58)
  >>>   
user-x64-sancov.shlib/obj/third_party/ulib/scudo/scudo.string_utils.cc.o:(scudo::Printf(char
 const*, ...))

Checking to see if this is could be an us problem or something this patch 
introduced.

For reference: 
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8880544155798188656/+/steps/build/0/steps/build_fuchsia/0/steps/ninja/0/steps/zircon/0/logs/raw_io.output_failure_raw_summary_/0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79698



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7d5bb94d7838: Run Coverage pass before other *San passes 
under new pass manager (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79698

Files:
  clang/lib/CodeGen/BackendUtil.cpp


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1002,6 +1002,15 @@
   const Triple ,
   const LangOptions ,
   const CodeGenOptions ) {
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+MPM.addPass(ModuleSanitizerCoveragePass(
+SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+CodeGenOpts.SanitizeCoverageBlacklistFiles));
+  }
+
   auto ASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
 MPM.addPass(RequireAnalysisPass());
 bool Recover = CodeGenOpts.SanitizeRecover.has(Mask);
@@ -1242,6 +1251,17 @@
 EntryExitInstrumenterPass(/*PostInlining=*/false)));
   });
 
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+PB.registerPipelineStartEPCallback([&](ModulePassManager ) {
+  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+  MPM.addPass(ModuleSanitizerCoveragePass(
+  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+  CodeGenOpts.SanitizeCoverageBlacklistFiles));
+});
+  }
+
   // Register callbacks to schedule sanitizer passes at the appropriate 
part of
   // the pipeline.
   // FIXME: either handle asan/the remaining sanitizers or error out
@@ -1326,15 +1346,6 @@
   }
 }
 
-if (CodeGenOpts.SanitizeCoverageType ||
-CodeGenOpts.SanitizeCoverageIndirectCalls ||
-CodeGenOpts.SanitizeCoverageTraceCmp) {
-  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
-  MPM.addPass(ModuleSanitizerCoveragePass(
-  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
-  CodeGenOpts.SanitizeCoverageBlacklistFiles));
-}
-
 if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
   bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
   MPM.addPass(HWAddressSanitizerPass(


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1002,6 +1002,15 @@
   const Triple ,
   const LangOptions ,
   const CodeGenOptions ) {
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+MPM.addPass(ModuleSanitizerCoveragePass(
+SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+CodeGenOpts.SanitizeCoverageBlacklistFiles));
+  }
+
   auto ASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
 MPM.addPass(RequireAnalysisPass());
 bool Recover = CodeGenOpts.SanitizeRecover.has(Mask);
@@ -1242,6 +1251,17 @@
 EntryExitInstrumenterPass(/*PostInlining=*/false)));
   });
 
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+PB.registerPipelineStartEPCallback([&](ModulePassManager ) {
+  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+  MPM.addPass(ModuleSanitizerCoveragePass(
+  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+  CodeGenOpts.SanitizeCoverageBlacklistFiles));
+});
+  }
+
   // Register callbacks to schedule sanitizer passes at the appropriate part of
   // the pipeline.
   // FIXME: either handle asan/the remaining sanitizers or error out
@@ -1326,15 +1346,6 @@
   }
 }
 
-if (CodeGenOpts.SanitizeCoverageType ||
-CodeGenOpts.SanitizeCoverageIndirectCalls ||
-CodeGenOpts.SanitizeCoverageTraceCmp) {
-  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
-  MPM.addPass(ModuleSanitizerCoveragePass(
-  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
-  CodeGenOpts.SanitizeCoverageBlacklistFiles));
-}
-
 if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
   bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
   MPM.addPass(HWAddressSanitizerPass(

[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79698



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79698: Run Coverage pass before other *San passes under new pass manager

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
aeubanks added reviewers: vitalybuka, leonardchan.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This fixes compiler-rt/test/msan/coverage-levels.cpp under the new pass manager 
(final check-msan test!).
Under the old pass manager, the coverage pass would run before the MSan pass. 
The opposite happened under the new pass manager. The MSan pass adds extra 
basic blocks, changing the number of coverage callbacks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79698

Files:
  clang/lib/CodeGen/BackendUtil.cpp


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1004,6 +1004,15 @@
   const Triple ,
   const LangOptions ,
   const CodeGenOptions ) {
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+MPM.addPass(ModuleSanitizerCoveragePass(
+SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+CodeGenOpts.SanitizeCoverageBlacklistFiles));
+  }
+
   auto ASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
 MPM.addPass(RequireAnalysisPass());
 bool Recover = CodeGenOpts.SanitizeRecover.has(Mask);
@@ -1244,6 +1253,17 @@
 EntryExitInstrumenterPass(/*PostInlining=*/false)));
   });
 
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+PB.registerPipelineStartEPCallback([&](ModulePassManager ) {
+  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+  MPM.addPass(ModuleSanitizerCoveragePass(
+  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+  CodeGenOpts.SanitizeCoverageBlacklistFiles));
+});
+  }
+
   // Register callbacks to schedule sanitizer passes at the appropriate 
part of
   // the pipeline.
   // FIXME: either handle asan/the remaining sanitizers or error out
@@ -1328,15 +1348,6 @@
   }
 }
 
-if (CodeGenOpts.SanitizeCoverageType ||
-CodeGenOpts.SanitizeCoverageIndirectCalls ||
-CodeGenOpts.SanitizeCoverageTraceCmp) {
-  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
-  MPM.addPass(ModuleSanitizerCoveragePass(
-  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
-  CodeGenOpts.SanitizeCoverageBlacklistFiles));
-}
-
 if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
   bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
   MPM.addPass(HWAddressSanitizerPass(


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1004,6 +1004,15 @@
   const Triple ,
   const LangOptions ,
   const CodeGenOptions ) {
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+MPM.addPass(ModuleSanitizerCoveragePass(
+SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+CodeGenOpts.SanitizeCoverageBlacklistFiles));
+  }
+
   auto ASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
 MPM.addPass(RequireAnalysisPass());
 bool Recover = CodeGenOpts.SanitizeRecover.has(Mask);
@@ -1244,6 +1253,17 @@
 EntryExitInstrumenterPass(/*PostInlining=*/false)));
   });
 
+  if (CodeGenOpts.SanitizeCoverageType ||
+  CodeGenOpts.SanitizeCoverageIndirectCalls ||
+  CodeGenOpts.SanitizeCoverageTraceCmp) {
+PB.registerPipelineStartEPCallback([&](ModulePassManager ) {
+  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
+  MPM.addPass(ModuleSanitizerCoveragePass(
+  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
+  CodeGenOpts.SanitizeCoverageBlacklistFiles));
+});
+  }
+
   // Register callbacks to schedule sanitizer passes at the appropriate part of
   // the pipeline.
   // FIXME: either handle asan/the remaining sanitizers or error out
@@ -1328,15 +1348,6 @@
   }
 }
 
-if (CodeGenOpts.SanitizeCoverageType ||
-CodeGenOpts.SanitizeCoverageIndirectCalls ||
-CodeGenOpts.SanitizeCoverageTraceCmp) {
-  auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
-  MPM.addPass(ModuleSanitizerCoveragePass(
-  SancovOpts, CodeGenOpts.SanitizeCoverageWhitelistFiles,
-