nickdesaulniers added a comment. In D90194#2377337 <https://reviews.llvm.org/D90194#2377337>, @rnk wrote:
> Given that only three tests fail when the `nossp` attribute gets added from > `-cc1` with no stack protector, I think it's reasonable to add it and skip > adding the extra enum. Sorry, when I said: In D90194#2354908 <https://reviews.llvm.org/D90194#2354908>, @nickdesaulniers wrote: >> An alternative approach to consider would be not adding a new enum, and >> making -stack-protector ALWAYS be specified when invoking cc1. I did not >> quantify the amount of changes to existing tests, but am happy to do so if >> reviewers would like. > > Making `-stack-protector` always be passed, and default to `0` rather than > being unspecified causes the following tests to fail: > > Failed Tests (3): > Clang :: Driver/cl-options.c > Clang :: Driver/fsanitize.c > Clang :: Driver/stack-protector.c > > which is much less than I was expecting. It's trivial to not create a new > enum value and always pass `-stack-protector <N>` along. On top of this > patch: I followed up that it wasn't as simple as 3 tests failing. I followed up with: In D90194#2360392 <https://reviews.llvm.org/D90194#2360392>, @nickdesaulniers wrote: >> It's trivial to not create a new enum value and always pass -stack-protector >> <N> along > > That was incorrect. Rebasing this on top of D90271 > <https://reviews.llvm.org/D90271> causes 49 test failures, because the diff I > posted above didn't actually remove the new enum or changes to > clang/lib/Frontend/CompilerInvocation.cpp. So I've answered my own question; > we should add this enum value. Let me rebase this differently then. I'm happy to rewrite this patch to not introduce the new enum, but it's going to be much larger than this patch due to churn in the tests. Lets me take a crack at it and I'll post it as a separate review. Then I think it would help illuminate the differences in approach and we can take our pick. Thanks all for the feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90194/new/ https://reviews.llvm.org/D90194 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
