yln added inline comments.

================
Comment at: 
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11
+
+// darwin only supports shared-libsan, so this should fail.
+// XFAIL: darwin
+ 
----------------
usama54321 wrote:
> yln wrote:
> > zixuw wrote:
> > > dmaclach wrote:
> > > > yln wrote:
> > > > > This should work, right?
> > > > No.. darwin should fail with the `-static-libsan` flag. This is the 
> > > > test that was failing and caused the rollback.
> > > I think @yln is suggesting using `REQUIRES: asan-static-runtime` instead 
> > > of `XFAIL: darwin`. I wasn't aware of that conditional but yeah that 
> > > should be better if it works.
> > I meant using `// REQUIRES: asan-static-runtime ` instead of `XFAIL: 
> > darwin` since it seems that we already have a lit feature for it.
> I think UNSUPPORTED: darwin makes the most sense here. I don't think lit 
> understands that REQUIRES: asan-static-runtime should result in skipping the 
> test on Darwin as it does not know about this dependency.
> I don't think lit understands that REQUIRES: asan-static-runtime should 
> result in skipping the test on Darwin as it does not know about this 
> dependency.

Actually, this was exactly my point.  We have other tests already marked with 
`REQUIRES: asan-static-runtime` and we should double check our changes don't 
affect these as well.

If LIT doesn't model this dependency yet, then we should make sure it does!  
And this test can act as a good "canary in the coal mine".

Please use `REQUIRES: asan-static-runtime` and make sure we understand and deal 
with any fallout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144672

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

Reply via email to