nickdesaulniers added inline comments.
================ Comment at: llvm/include/llvm/Bitcode/LLVMBitCodes.h:610 + // TODO: reorder + ATTR_KIND_NO_STACK_PROTECT = 70, ATTR_KIND_STACK_PROTECT = 26, ---------------- nickdesaulniers wrote: > any comments from reviewers before I go and do a tedious reordering of these > enum values? Oof, it looks like inserting `ATTR_KIND_NO_STACK_PROTECT = 26` then reordering the rest causes the following tests to fail: ``` Failed Tests (17): LLVM :: Bitcode/DIExpression-aggresult.ll LLVM :: Bitcode/DITemplateParameter-5.0.ll LLVM :: Bitcode/PR23310.test LLVM :: Bitcode/apple-clang-700-compat.test LLVM :: Bitcode/attributes-3.3.ll LLVM :: Bitcode/case-ranges-3.3.ll LLVM :: Bitcode/compatibility-3.6.ll LLVM :: Bitcode/compatibility-3.7.ll LLVM :: Bitcode/compatibility-3.8.ll LLVM :: Bitcode/compatibility-3.9.ll LLVM :: Bitcode/compatibility-4.0.ll LLVM :: Bitcode/compatibility-5.0.ll LLVM :: Bitcode/compatibility-6.0.ll LLVM :: Bitcode/drop-debug-info.3.5.ll LLVM :: Bitcode/invalid-functionptr-align.ll LLVM :: Object/nm-bitcode.test LLVM :: tools/llvm-nm/X86/IRobj.test ``` under the assumption that new fn attr's should get monotonically increasing numbers, I'll stick this new one at the end. That should preserve bitcode compatibility, at the cost of the similar function routines not being grouped in successive order (which is a stylistic or subjective complaint). ================ Comment at: llvm/lib/IR/Attributes.cpp:1904-1905 // If upgrading the SSP attribute, clear out the old SSP Attributes first. // Having multiple SSP attributes doesn't actually hurt, but it adds useless // clutter to the IR. AttrBuilder OldSSPAttr; ---------------- I should update this comment to say they're not mutually exclusive. ================ Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1682 + // protector. + if (Caller->hasFnAttribute(Attribute::NoStackProtect)) + if (CalledFunc->hasFnAttribute(Attribute::StackProtect) || ---------------- void wrote: > Should we check whether the function with nossp needs an ssp before issuing > the error? No; it should never be the case that "the function with nossp needs an ssp" since `nossp` and `ssp` are now mutually exclusive, `nossp` means "I know what I'm doing, and accept the consequences," and `StackProtector::RequiresStackProtector` will always return `false` in that case after my changes (even if it was a static method we could easily access, and not a private method of a separate pass). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87956/new/ https://reviews.llvm.org/D87956 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
