[PATCH] D67545: [clang-tidy] Added DefaultOperatorNewCheck.

Wed Sep 18 07:49:20 PDT 2019

balazske marked an inline comment as done.
balazske 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:
> 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).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67545/new/

https://reviews.llvm.org/D67545