lebedev.ri marked an inline comment as done. lebedev.ri added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:51 + // The alignment used by default 'operator new' (in bits). + const unsigned DefaultAlignment = Context.getTargetInfo().getNewAlign(); + ---------------- aaron.ballman wrote: > balazske wrote: > > aaron.ballman wrote: > > > aaron.ballman wrote: > > > > martong wrote: > > > > > aaron.ballman wrote: > > > > > > martong wrote: > > > > > > > martong wrote: > > > > > > > > aaron.ballman wrote: > > > > > > > > > lebedev.ri wrote: > > > > > > > > > > martong wrote: > > > > > > > > > > > martong wrote: > > > > > > > > > > > > What is the difference between "default" and > > > > > > > > > > > > "fundamental" alignment? Are they the same? Can they > > > > > > > > > > > > differ in any architecture? > > > > > > > > > > > > > > > > > > > > > > > > https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM57-CPP.+Avoid+using+default+operator+new+for+over-aligned+types > > > > > > > > > > > > Here there is no wording of "default alignment" only > > > > > > > > > > > > "fundamental alignment" is mentioned. Based on this I'd > > > > > > > > > > > > call this as `FundamentalAligment`. > > > > > > > > > > > > What is the difference between "default" and > > > > > > > > > > > > "fundamental" alignment? Are they the same? > > > > > > > > > > > > > > > > > > > > > > `fundamental alignment` of any type is the alignment of > > > > > > > > > > > std::max_align_t. I.e. `alignof(std::max_align_t)`. > > > > > > > > > > > See C++17 6.11.2. > > > > > > > > > > > > > > > > > > > > > > On the other hand, default alignment is the value in > > > > > > > > > > > `__STDCPP_DEFAULT_NEW_ALIGNMENT__` which may be > > > > > > > > > > > predefined with `fnew-alignment` > > > > > > > > > > > See https://www.bfilipek.com/2019/08/newnew-align.html > > > > > > > > > > > > > > > > > > > > > > These values can differ: > > > > > > > > > > > https://wandbox.org/permlink/yIwjiNMw9KyXEQan > > > > > > > > > > > > > > > > > > > > > > Thus, I think we should use the fundamental alignment > > > > > > > > > > > here, not the default alignment. > > > > > > > > > > > So, `getNewAlign()` does not seem right to me. > > > > > > > > > > > @aaron.ballman What do you think? > > > > > > > > > > > Thus, I think we should use the fundamental alignment > > > > > > > > > > > here, not the default alignment. > > > > > > > > > > > > > > > > > > > > I have the exact opposite view. > > > > > > > > > > If as per `getNewAlign()` the alignment would be okay, why > > > > > > > > > > should we not trust it? > > > > > > > > > The comment on `getNewAlign()` is: > > > > > > > > > ``` > > > > > > > > > /// Return the largest alignment for which a suitably-sized > > > > > > > > > allocation with > > > > > > > > > /// '::operator new(size_t)' is guaranteed to produce a > > > > > > > > > correctly-aligned > > > > > > > > > /// pointer. > > > > > > > > > ``` > > > > > > > > > I read that as saying any alignment larger than what is > > > > > > > > > returned by `getNewAlign()` must call the over-aligned > > > > > > > > > operator new variant in C++17 if available. So if the actual > > > > > > > > > call target doesn't have an alignment specifier, it's > > > > > > > > > probably getting the alignment wrong and would be worth > > > > > > > > > diagnosing on. > > > > > > > > > I have the exact opposite view. > > > > > > > > > If as per getNewAlign() the alignment would be okay, why > > > > > > > > > should we not trust it? > > > > > > > > > > > > > > > > That could lead to a false positive diagnostic if > > > > > > > > `-fnew-alignment=8` and `alignas(16)` , because > > > > > > > > `alignof(max_align_t)` is still 16. > > > > > > > > > > > > > > > > See the definidion of `getNewAlign()` which will return with 8 > > > > > > > > in this case I suppose: > > > > > > > > ``` > > > > > > > > unsigned getNewAlign() const { > > > > > > > > return NewAlign ? NewAlign : std::max(LongDoubleAlign, > > > > > > > > LongLongAlign); > > > > > > > > } > > > > > > > > ``` > > > > > > > > So if the actual call target doesn't have an alignment > > > > > > > > specifier, it's probably getting the alignment wrong and would > > > > > > > > be worth diagnosing on. > > > > > > > > > > > > > > I agree, but then we are implementing a checker which is > > > > > > > different from the description given in cert-mem57. > > > > > > > So it is not a CERT checker anymore, perhaps we should rename > > > > > > > then. > > > > > > > There is no mention of __STDCPP_DEFAULT_NEW_ALIGNMENT__ in > > > > > > > https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM57-CPP.+Avoid+using+default+operator+new+for+over-aligned+types > > > > > > > It clearly references the "fundamental alignement". > > > > > > Why do you believe that to be a false positive? That seems like > > > > > > exactly the behavior we'd want -- if the user says that their > > > > > > allocation function guarantees a particular max alignment by using > > > > > > `-fnew-alignment`, we should honor that. > > > > > > Why do you believe that to be a false positive? That seems like > > > > > > exactly the behavior we'd want -- if the user says that their > > > > > > allocation function guarantees a particular max alignment by using > > > > > > -fnew-alignment, we should honor that. > > > > > > > > > > Okay, giving it more thought, that makes perfect sense. > > > > > Anyway, thanks for trying to understand my concerns :) > > > > That's because the CERT rule was written to target C++14 and earlier, > > > > which did not have `__STDCPP_DEFAULT_NEW_ALIGNMENT__`. > > > > > > > > We can solve this in one of two ways: don't enable the check in C++17 > > > > mode, or do the right thing in C++17 mode. I think we should do the > > > > right thing, which is to check which overload is selected (if the > > > > aligned overload is selected, we don't diagnose because it's doing the > > > > right thing for the user) and compare against `getNewAlign()` (if the > > > > alignment requested is stricter than what we can guarantee through > > > > `getNewAlign()` and we've verified we're not calling an aligned > > > > overload, there is a real chance the pointer value will be incorrectly > > > > aligned). To me, that meets the spirit of what the CERT rule is trying > > > > to convey while still being useful in C++17. > > > > Okay, giving it more thought, that makes perfect sense. > > > > Anyway, thanks for trying to understand my concerns :) > > > > > > Thank you for the good discussion on them! > > I am not sure about if really any warning is needed in C++17. Because if > > the non-aligned `operator new` is selected it is already an user-provided > > one (the system-provided allocation function should take the alignment > > argument). And for user-defined `operator new` we do not want any warning > > (even if it looks like wrong but may be it is not). > I think this makes sense. If the user had replaced the global operator new > but not the aligned one, the aligned one would still be found by overload > resolution when it matters. So it seems fine to ignore C++17 for the check > for now. Tangentially-related rant: The `operator new`/`malloc`&friend alignment assumptions question isn't new, it was brought up on the list several times, and i think it will be approached once more. Right now clang, even though it knows about those alignments, it does not annotate them (`operator new`, ...) with alignment assumption, thus both pessimizing middle-end (knowing that some pointer has some big alignment is useful!), and "disabling" the appropriate UBSan check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67545/new/ https://reviews.llvm.org/D67545 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
