aaron.ballman 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(); + ---------------- 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. 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
