yln accepted this revision. 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: > > 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. > Sorry for my earlier comment. @yln is correct, and we should use REQUIRES: > asan-static-runtime. I double checked, and this already works as expected on > darwin, i.e. these tests are unsupported on darwin. So you should not need to > do anything apart from adding the REQUIRES in the test. > [...] I double checked, and this already works as expected on darwin, i.e. > these tests are unsupported on darwin. Thanks Usama! Patch LGTM assuming we switch to using `REQUIRES: asan-static-runtime`. Thanks for seeing this through! :) 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
