aligned 'new'

Clang Frontend
1

Hi all,

I am thinking of the ability of operator new to return memory aligned according to the alignment of the type passed to it (at least of AltiVec vector types whose proper alignment is essential).
The proposed implementation is:
1) clang: add -fstrict-aligned option. When the option is set it cause clang to define the built-in macro "__STRICT_ALIGNED"
2) libcxx: add additional placement new declarations to libcxx\include\new with the additional argument - alignment. Make this declarations conditionalized on the "__STRICT_ALIGNED". Add new definitions to libcxx\src\new.cpp.
3) clang: if -fstrict-aligned option is set, when clang sees a new of a type which either has an inherent alignment, or has an explicit alignment specified by __attribute__((__aligned__(x))), it will transform the standard new call to a call to the aligned version of new.

Am i on the right way? Any thoughts?

2

I don't think that this is the right way to go. Instead, you'd want "new foo" to automatically detect that the alignment of "foo" is greater than the alignment guaranteed by the system malloc. In this case, and if using the default operator new, you'd generate a call to memalign instead of to operator new (something like that).

It isn't clear to me how generally useful this is though, and we tend to avoid adding tons of language extensions to Clang.

-Chris

3

Specifically, extensions that are not already available in other compilers. We seem to have an affinity for emulating other compilers weird quirks and extensions :slight_smile:

-Chris

4

I just found this option in ppu-lv2-gcc from the Sony Cell SDK :slight_smile:

5

Attached is the the patch that substitutes calls to default new operators with calls to functions - aligned analogs of default new operators. This functions may live in the clang's mm_malloc.h header where aligned malloc lives. The patch is partial, it does not affect deletes and do not contain substituting functions yet. Please review and direct!

aligned_new_part1.patch (6.8 KB)

6

Ping!

7

Hi,

here is the updated patch that substitute both news and deletes, please review!

aligned_new_part1.patch (11.4 KB)

8

Final patch with aligned functions added, please review!

aligned_new.patch (15.3 KB)

9

Ping!

10

Ping!

11

Hi Anton,

Can you include the most recent patch?

-Chris

12

Attached

aligned_new_delete.patch (15.3 KB)

13

I have a few specific comments about the patch, and a meta-comment below.

+bool FunctionDecl::isAlignedSubstitutionOfNewDelete(bool CheckNew,
+ bool CheckNewArr,
+ bool CheckDelete,
+ bool CheckDeleteArr) const {

14

Thanks a lot for the review!

+bool FunctionDecl::isAlignedSubstitutionOfNewDelete(bool CheckNew,
+ bool CheckNewArr,
+ bool CheckDelete,
+ bool CheckDeleteArr) const {
+
+ if (!getASTContext().getLangOptions().StrictAligned)
+ return false;
+
+ std::string Name = getDeclName().getAsString();
+
+ return ((CheckNew&& !Name.compare("__aligned_new")) ||
+ (CheckNewArr&& !Name.compare("__aligned_new_arr")) ||
+ (CheckDelete&& !Name.compare("__aligned_delete")) ||
+ (CheckDeleteArr&& !Name.compare("__aligned_delete_arr")));
+}

DeclarationName::getAsString() is not an efficient operation. Please use getAsIdentifierInfo() and compare against that. If this routine will be called often, it would be far better if this comparison where a pointer comparison (against a known IdentifierInfo*) rather than a string comparison.

done

+
+// Alligned analogs of standard library operator new functions declared
+// in the<new> header
+void *__aligned_new(std::size_t size, std::size_t align) throw (std::bad_alloc)

This is a non-static, non-inline function definition in a header, which can't be right.

oops.. fixed

Besides, shouldn't the default implementation for this function be part of the C++ support library, where the default operator new is defined?

Putting this functions to clang header makes the feature independent of library used

+ // Call to a new operator was replaced with the call to __aligned_new or
+ // __aligned_new_arr function.
+ // Add alignment argument in front of placement arguments
+ if (getLangOptions().StrictAligned&&
+ OperatorNew->isAlignedSubstitutionOfNewDelete(
+ /*new*/true, /*new*/true, /*delete*/false, /*delete*/false)) {
+
+ unsigned Alignment =
+ Context.getTypeAlignInChars(AllocType).getQuantity();
+
+ Expr* AlignmentArg =
+ IntegerLiteral::Create(Context, llvm::APInt(
+ Context.Target.getIntWidth(), Alignment),
+ Context.IntTy, SourceLocation());

Since the alignment parameters have type std::size_t, shouldn't the IntegerLiteral be created with the size type?

fixed

+ // nothrow new
+ if (const ReferenceType *Ref = secontParamType->getAs<ReferenceType>())
+ if (const RecordType *Rec = Ref->getPointeeType()->getAs<RecordType>())
+ if (Rec->getDecl()->getDeclName().getAsString() == "nothrow_t")
+ if (NamespaceDecl *Namespace =
+ dyn_cast<NamespaceDecl>(Rec->getDecl()->getParent()))
+ if (Namespace->getDeclName().getAsString() == "std")
+ return true;

String comparisons on type names aren't something we do in Sema except in extreme circumstances. Please find a better way to identify nothrow.

done

@@ -1282,10 +1354,54 @@

      // Didn't find a member overload. Look for a global one.
      DeclareGlobalNewDelete();
      DeclContext *TUDecl = Context.getTranslationUnitDecl();
+
      if (FindAllocationOverload(StartLoc, Range, NewName,&AllocArgs[0],
                            AllocArgs.size(), TUDecl, /*AllowMissing=*/false,
                            OperatorNew))
        return true;
+
+ // replace calls to allocation functions defined in the<new> header
+ // with calls to its aligned analogs
+ if (getLangOptions().StrictAligned&&
+ isDefaultLibraryAllocationFunction(OperatorNew)) {
+
+ OperatorNew = 0;
+
+ DeclarationName AlignedNewName(
+&Context.Idents.get(IsArray ? "__aligned_new_arr" : "__aligned_new"));
+
+ DeclarationName AlignedDeleteName(
+&Context.Idents.get(IsArray ? "__aligned_delete_arr"
+ : "__aligned_delete"));
+
+ // construct additional parameter - alignment
+ IntegerLiteral AlignmentArg(Context, llvm::APInt::getNullValue(
+ Context.Target.getPointerWidth(0)),
+ Context.getSizeType(),
+ SourceLocation());
+
+ llvm::SmallVector<Expr*, 8> ArgsPlusAlign(AllocArgs);
+
+ // insert alignment parameter just after the Size parameter
+ ArgsPlusAlign.insert(ArgsPlusAlign.begin()+1,&AlignmentArg);
+
+ if (FindAllocationOverload(StartLoc, Range, AlignedNewName,
+&ArgsPlusAlign[0], ArgsPlusAlign.size(), TUDecl,
+ /*AllowMissing=*/false, OperatorNew))
+ return true;

There's no fallback here, which seems to imply that if the call to __aligned_new/__aligned_new_arr/etc. fails, we won't end up trying to the standard-mandated operator new/operator new/etc. Is that really the intended behavior? It seems like fairly significant breakage to me.

If the call to __aligned_... fails we end up with an error ('no matching function for call to ...'). This behavior seem correct to me as specifying -fstrict-aligned option we expect our data to be aligned properly.

- FunctionDecl *Fn = cast<FunctionDecl>(D);
- AddOverloadCandidate(Fn, Alloc.getPair(), Args, NumArgs, Candidates,
- /*SuppressUserConversions=*/false);
+ if (FunctionDecl *Fn = dyn_cast<FunctionDecl>(D))
+ AddOverloadCandidate(Fn, Alloc.getPair(), Args, NumArgs, Candidates,
+ /*SuppressUserConversions=*/false);
    }

Why is this change necessary? Is seems like an invariant has been broken, if this change did anything.

I got a crash here when gave clang bad sources to compile and clang treated __aligned... function as variable declaration while trying to recover from errors.

Now, the meta-level point: this is a significant extension to Clang, so it needs proper documentation, a specification, a test suite, etc., as described in

  http://clang.llvm.org/get_involved.html

You mentioned that this is a pre-existing feature for ppu-lv2-gcc from the Sony Cell SDK, which likely means that the design of the feature is already fairly constrained. That said, we need to understand the design of the feature before we can evaluate the implementation, and I still don't have a good sense of how the feature is supposed to work. More documentation and examples would help a lot.

added the test.

That is how the feature is designed in ppu-lv2-gcc:
giving -fstrict-align option to ppu-lv2-gcc does the following:
1) It cause ppu-lv2-gcc to define the built-in macro "__STRICT_ALIGNED".
2) The PS3 header "new" has some new declarations for new and delete operators (standard, nothrow and placement, both simple and array versions) that take an additional size_t parameter for alignment. These are conditionalized on the __STRICT_ALIGNED flag.
3) If this option is set, when ppu-lv2-gcc sees a new or delete of a type which either has an inherent alignment, or has an explicit alignment specified by __attribute__((__aligned__(x))), it will transform the standard new or delete call to a call to the aligned version of new or delete.

Please review and comment.
Tell if I can commit anything from the patch.

aligned_new_v1.patch (15.6 KB)

aligned_new.cpp (1.44 KB)

15

This is a terrible choice of name for this command-line option. If I had to come up with a list of ten things that "strict-align" means, I don't think "make operator new take an alignment argument" would be on it at all. Since this feature has been implemented in precisely one previous vendor-specific frontend, I think it's worthwhile to establish a better precedent.

I would suggest something like -foperator-new-alignment.

John.

16

Why do you need to change the language for that at all?
At least for UNIX-like systems, you could just use a call to
posix_memalign with your size,alignment pair and call the placement new
on the result. Wrap it around in one or two macros (instance vs array)
and it is done.

Joerg

17

Why do you need to change the language for that at all?
At least for UNIX-like systems, you could just use a call to
posix_memalign with your size,alignment pair and call the placement new
on the result. Wrap it around in one or two macros (instance vs array)
and it is done.

Because calls to new can be inserted by a compiler? :slight_smile:

18

...which would just use the default settings and therefore doesn't
really matter.

Joerg

19

[Dropping Chris; he doesn't need to be CC'd on this]

Thanks a lot for the review!

+bool FunctionDecl::isAlignedSubstitutionOfNewDelete(bool CheckNew,
+ bool CheckNewArr,
+ bool CheckDelete,
+ bool CheckDeleteArr) const {
+
+ if (!getASTContext().getLangOptions().StrictAligned)
+ return false;
+
+ std::string Name = getDeclName().getAsString();
+
+ return ((CheckNew&& !Name.compare("__aligned_new")) ||
+ (CheckNewArr&& !Name.compare("__aligned_new_arr")) ||
+ (CheckDelete&& !Name.compare("__aligned_delete")) ||
+ (CheckDeleteArr&& !Name.compare("__aligned_delete_arr")));
+}

DeclarationName::getAsString() is not an efficient operation. Please use getAsIdentifierInfo() and compare against that. If this routine will be called often, it would be far better if this comparison where a pointer comparison (against a known IdentifierInfo*) rather than a string comparison.

done

+
+// Alligned analogs of standard library operator new functions declared
+// in the<new> header
+void *__aligned_new(std::size_t size, std::size_t align) throw (std::bad_alloc)

This is a non-static, non-inline function definition in a header, which can't be right.

oops.. fixed

Besides, shouldn't the default implementation for this function be part of the C++ support library, where the default operator new is defined?

Putting this functions to clang header makes the feature independent of library used

Yes, but it has several unfortunate side effects:

  (1) We'll end up with copies of these functions in every .o file that uses new or delete, which is likely to cause some serious code bloat
  (2) Unlike with the default new/new/delete/delete operators provided by the C++ implementation, these 'aligned' versions are not replaceable by the user because their definitions are internal to each .o file

There are other ways to achieve library independence. For example, the definitions could be in a separate library that we link against when -fstrict-aligned is provided, and the declarations themselves could be synthesized by the compiler when needed (e.g., as a library built-in).

@@ -1282,10 +1354,54 @@

     // Didn't find a member overload. Look for a global one.
     DeclareGlobalNewDelete();
     DeclContext *TUDecl = Context.getTranslationUnitDecl();
+
     if (FindAllocationOverload(StartLoc, Range, NewName,&AllocArgs[0],
                           AllocArgs.size(), TUDecl, /*AllowMissing=*/false,
                           OperatorNew))
       return true;
+
+ // replace calls to allocation functions defined in the<new> header
+ // with calls to its aligned analogs
+ if (getLangOptions().StrictAligned&&
+ isDefaultLibraryAllocationFunction(OperatorNew)) {
+
+ OperatorNew = 0;
+
+ DeclarationName AlignedNewName(
+&Context.Idents.get(IsArray ? "__aligned_new_arr" : "__aligned_new"));
+
+ DeclarationName AlignedDeleteName(
+&Context.Idents.get(IsArray ? "__aligned_delete_arr"
+ : "__aligned_delete"));
+
+ // construct additional parameter - alignment
+ IntegerLiteral AlignmentArg(Context, llvm::APInt::getNullValue(
+ Context.Target.getPointerWidth(0)),
+ Context.getSizeType(),
+ SourceLocation());
+
+ llvm::SmallVector<Expr*, 8> ArgsPlusAlign(AllocArgs);
+
+ // insert alignment parameter just after the Size parameter
+ ArgsPlusAlign.insert(ArgsPlusAlign.begin()+1,&AlignmentArg);
+
+ if (FindAllocationOverload(StartLoc, Range, AlignedNewName,
+&ArgsPlusAlign[0], ArgsPlusAlign.size(), TUDecl,
+ /*AllowMissing=*/false, OperatorNew))
+ return true;

There's no fallback here, which seems to imply that if the call to __aligned_new/__aligned_new_arr/etc. fails, we won't end up trying to the standard-mandated operator new/operator new/etc. Is that really the intended behavior? It seems like fairly significant breakage to me.

If the call to __aligned_... fails we end up with an error ('no matching function for call to ...'). This behavior seem correct to me as specifying -fstrict-aligned option we expect our data to be aligned properly.

Does this actually break existing C++ code that overloads operators new/new/delete/delete, or am I missing something? If it does break code, is that "by design" or "by accident"?

Now, the meta-level point: this is a significant extension to Clang, so it needs proper documentation, a specification, a test suite, etc., as described in

  http://clang.llvm.org/get_involved.html

You mentioned that this is a pre-existing feature for ppu-lv2-gcc from the Sony Cell SDK, which likely means that the design of the feature is already fairly constrained. That said, we need to understand the design of the feature before we can evaluate the implementation, and I still don't have a good sense of how the feature is supposed to work. More documentation and examples would help a lot.

added the test.

Okay, thanks.

That is how the feature is designed in ppu-lv2-gcc:
giving -fstrict-align option to ppu-lv2-gcc does the following:
1) It cause ppu-lv2-gcc to define the built-in macro "__STRICT_ALIGNED".
2) The PS3 header "new" has some new declarations for new and delete operators (standard, nothrow and placement, both simple and array versions) that take an additional size_t parameter for alignment. These are conditionalized on the __STRICT_ALIGNED flag.

If the PS3 header <new> has the declarations of the aligned new/delete operators now, why is your patch adding these operators to Clang's mm_alloc.h? Isn't the precedent already there to make sure <new> has these declarations, which is most likely the better answer?

3) If this option is set, when ppu-lv2-gcc sees a new or delete of a type which either has an inherent alignment, or has an explicit alignment specified by __attribute__((__aligned__(x))), it will transform the standard new or delete call to a call to the aligned version of new or delete.

Every type has alignment, so you're saying that every "standard" new/delete/new/delete expression calls into the aligned version of new/delete/new/delete. And you're only talking about the global operators new/delete/new/delete that have one of the forms described in <new>; this replacement doesn't apply for operators with different signatures from those in <new> or for those operators when they're defined at class scope. Is the omission of support for allocation/deallocation functions at class scope intentional? If so, the compiler should emit an error if someone does write one of these operators at class scope, because they're useless (and accepting them is misleading). If not, then we're missing code to support that case.

Some more detailed comments to follow, but I'm having a lot of trouble with this feature. I understand that the design of the feature is not really up for debate, because we're implementing an extension that was defined by another compiler, and I can live with that. That said, we need to have a specification of this feature that users and compiler writers alike can use to understand how this feature is supposed to work, because we don't all have access to a copy of ppu-lv2-gcc to act as an oracle for how this feature behaves. The specification needs to go into Clang's language extensions page, so people know it is there and usable:

  http://clang.llvm.org/docs/LanguageExtensions.html

Now, for the minor comments:

Index: include/clang/Basic/LangOptions.h

20

Here is an updated patch with lots of changes. Tried to make it of higher quality. Please review.

aligned_new_test.cpp - new test from clang\test\CodeGenCXX\
aligned_new.h - header with new declarations from clang\lib\Headers\
HelperLibs.zip - files for creating library. Placed it to clang\lib\

Putting this functions to clang header makes the feature independent of library used

Yes, but it has several unfortunate side effects:

   (1) We'll end up with copies of these functions in every .o file that uses new or delete, which is likely to cause some serious code bloat
   (2) Unlike with the default new/new/delete/delete operators provided by the C++ implementation, these 'aligned' versions are not replaceable by the user because their definitions are internal to each .o file

There are other ways to achieve library independence. For example, the definitions could be in a separate library that we link against when -fstrict-aligned is provided, and the declarations themselves could be synthesized by the compiler when needed (e.g., as a library built-in).

Made standard non-placement declarations synthesized by the compiler as it is made for standard non-placement new/new/delete/delete. Moved the definitions to separate library

@@ -1282,10 +1354,54 @@

      // Didn't find a member overload. Look for a global one.
      DeclareGlobalNewDelete();
      DeclContext *TUDecl = Context.getTranslationUnitDecl();
+
      if (FindAllocationOverload(StartLoc, Range, NewName,&AllocArgs[0],
                            AllocArgs.size(), TUDecl, /*AllowMissing=*/false,
                            OperatorNew))
        return true;
+
+ // replace calls to allocation functions defined in the<new> header
+ // with calls to its aligned analogs
+ if (getLangOptions().StrictAligned&&
+ isDefaultLibraryAllocationFunction(OperatorNew)) {
+
+ OperatorNew = 0;
+
+ DeclarationName AlignedNewName(
+&Context.Idents.get(IsArray ? "__aligned_new_arr" : "__aligned_new"));
+
+ DeclarationName AlignedDeleteName(
+&Context.Idents.get(IsArray ? "__aligned_delete_arr"
+ : "__aligned_delete"));
+
+ // construct additional parameter - alignment
+ IntegerLiteral AlignmentArg(Context, llvm::APInt::getNullValue(
+ Context.Target.getPointerWidth(0)),
+ Context.getSizeType(),
+ SourceLocation());
+
+ llvm::SmallVector<Expr*, 8> ArgsPlusAlign(AllocArgs);
+
+ // insert alignment parameter just after the Size parameter
+ ArgsPlusAlign.insert(ArgsPlusAlign.begin()+1,&AlignmentArg);
+
+ if (FindAllocationOverload(StartLoc, Range, AlignedNewName,
+&ArgsPlusAlign[0], ArgsPlusAlign.size(), TUDecl,
+ /*AllowMissing=*/false, OperatorNew))
+ return true;

There's no fallback here, which seems to imply that if the call to __aligned_new/__aligned_new_arr/etc. fails, we won't end up trying to the standard-mandated operator new/operator new/etc. Is that really the intended behavior? It seems like fairly significant breakage to me.

If the call to __aligned_... fails we end up with an error ('no matching function for call to ...'). This behavior seem correct to me as specifying -fstrict-aligned option we expect our data to be aligned properly.

Does this actually break existing C++ code that overloads operators new/new/delete/delete, or am I missing something? If it does break code, is that "by design" or "by accident"?

No, calls to standard new/new/delete/delete, either replaced or not, will be substituted, calls to overloaded new/new/delete/delete will remain unchanged. Calls to new/new/delete/delete declared in the class scope remain unchanged regardless of the signature.

That is how the feature is designed in ppu-lv2-gcc:
giving -fstrict-align option to ppu-lv2-gcc does the following:
1) It cause ppu-lv2-gcc to define the built-in macro "__STRICT_ALIGNED".
2) The PS3 header "new" has some new declarations for new and delete operators (standard, nothrow and placement, both simple and array versions) that take an additional size_t parameter for alignment. These are conditionalized on the __STRICT_ALIGNED flag.

If the PS3 header<new> has the declarations of the aligned new/delete operators now, why is your patch adding these operators to Clang's mm_alloc.h? Isn't the precedent already there to make sure<new> has these declarations, which is most likely the better answer?

I want the declarations live in the <new> header, but I want the feature be independent of the library. Adding <new> header to clang leads to ambiguity. Moved the declarations temporarily to separate header aligned_new.h. It will not be included often as standard non-placement declarations are now synthesized by clang. I can make the rest of the declarations also be synthesized and then there will be no need to include aligned_new.h at all

3) If this option is set, when ppu-lv2-gcc sees a new or delete of a type which either has an inherent alignment, or has an explicit alignment specified by __attribute__((__aligned__(x))), it will transform the standard new or delete call to a call to the aligned version of new or delete.

Every type has alignment, so you're saying that every "standard" new/delete/new/delete expression calls into the aligned version of new/delete/new/delete. And you're only talking about the global operators new/delete/new/delete that have one of the forms described in<new>; this replacement doesn't apply for operators with different signatures from those in<new> or for those operators when they're defined at class scope. Is the omission of support for allocation/deallocation functions at class scope intentional? If so, the compiler should emit an error if someone does write one of these operators at class scope, because they're useless (and accepting them is misleading). If not, then we're missing code to support that case.

Calls to new/new/delete/delete declared in the class scope are all left unchanged. No errors or warnings are generated. That is how ppu-lv2-gcc does. Do you want me to change the behavior?

Some more detailed comments to follow, but I'm having a lot of trouble with this feature. I understand that the design of the feature is not really up for debate, because we're implementing an extension that was defined by another compiler, and I can live with that. That said, we need to have a specification of this feature that users and compiler writers alike can use to understand how this feature is supposed to work, because we don't all have access to a copy of ppu-lv2-gcc to act as an oracle for how this feature behaves. The specification needs to go into Clang's language extensions page, so people know it is there and usable:

  http://clang.llvm.org/docs/LanguageExtensions.html

Updated LanguageExtensions.html

Now, for the minor comments:

Index: include/clang/Basic/LangOptions.h

--- include/clang/Basic/LangOptions.h (revision 138994)
+++ include/clang/Basic/LangOptions.h (working copy)
@@ -140,6 +140,8 @@

    unsigned MRTD : 1; // -mrtd calling convention
    unsigned DelayedTemplateParsing : 1; // Delayed template parsing

+ unsigned StrictAligned : 1; // Enforce aligned attribute with 'operator new'
+
  private:
    // We declare multibit enums as unsigned because MSVC insists on making enums
    // signed. Set/Query these values using accessors.

Every LangOptions flag needs to be serialized/de-serialized and checked by the ASTWriter and ASTReader.

Also, StrictAligned needs to be initialized.

Done

Index: include/clang/Driver/CC1Options.td

--- include/clang/Driver/CC1Options.td (revision 138994)
+++ include/clang/Driver/CC1Options.td (working copy)
@@ -433,6 +433,8 @@

    HelpText<"Disable implicit builtin knowledge of functions">;
  def faltivec : Flag<"-faltivec">,
    HelpText<"Enable AltiVec vector initializer syntax">;
+def fstrict_aligned : Flag<"-fstrict-aligned">,
+ HelpText<"Enforce aligned attribute with 'operator new'">;
  def fno_access_control : Flag<"-fno-access-control">,
    HelpText<"Disable C++ access control">;
  def fno_assume_sane_operator_new : Flag<"-fno-assume-sane-operator-new">,

This is a -cc1-level flag. I assume that you also want it to work in the Clang driver? You'll need to update the driver, too.

Done

Index: lib/AST/Decl.cpp

--- lib/AST/Decl.cpp (revision 138994)
+++ lib/AST/Decl.cpp (working copy)
@@ -1533,7 +1533,31 @@

           getIdentifier()->isStr("main");
  }

+bool FunctionDecl::isAlignedSubstitutionOfNewDelete(bool CheckNew,
+ bool CheckNewArr,
+ bool CheckDelete,
+ bool CheckDeleteArr) const {
+
+ if (!getASTContext().getLangOptions().StrictAligned)
+ return false;
+
+ IdentifierInfo *II = getDeclName().getAsIdentifierInfo();
+
+ return (II&& ((CheckNew&& II->isStr("__aligned_new")) ||
+ (CheckNewArr&& II->isStr("__aligned_new_arr")) ||
+ (CheckDelete&& II->isStr("__aligned_delete")) ||
+ (CheckDeleteArr&& II->isStr("__aligned_delete_arr"))));
+}

isStr() is inefficient. You'll need to cache the IdentifierInfos for these four strings and perform pointer comparisons against II.

I also find this API to be a bit strange… why not call this classifyAlignedSubstitutionOfNewDelete() and return a value of enumeration type that tells us which one it is?

Done

  bool FunctionDecl::isReservedGlobalPlacementOperator() const {
+
+ // calls to new/delete were substituted by calls to it's aligned analogs
+ if (isAlignedSubstitutionOfNewDelete(
+ /*new*/true, /*new*/true, /*delete*/true, /*delete*/true))
+ return false;
+ //FIXME: maybe add logic for detecting aligned analogs of reserved global
+ // placement operators

Why is this isAlignedSubstitutionOfNewDelete check even necessary? If you're changing the contract of FunctionDecl::isReservedGlobalPlacementOperator() to allow functions whose name isn't a C++ new/delete/new/delete operator, that's fine, but just return false if

Done

  getDeclName().getNameKind() != DeclarationName::CXXOperatorName

@@ -1836,6 +1839,9 @@
      else
        Opts.ObjCXXARCStandardLibrary = (ObjCXXARCStandardLibraryKind)Library;
    }
+
+ if(Args.hasArg(OPT_fstrict_aligned))
+ Opts.Includes.push_back("mm_malloc.h");
  }

Architecturally, I'm strongly opposed to auto-including headers like this. Either the user should have to include a header manually (e.g.,<new>) or the compiler should synthesize the declarations it needs on demand.

Done

What is with the option name John was talking about? Should i change the

aligned_new.patch (36.5 KB)

aligned_new_test.cpp (1.98 KB)

aligned_new.h (2.47 KB)

HelperLibs.zip (3.82 KB)