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

[Aaron Ballman via Phabricator via cfe-commits]

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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67545