[cfe-dev] aligned 'new'
Douglas Gregor
dgregor at apple.com
Thu Sep 8 12:13:15 PDT 2011
[Dropping Chris; he doesn't need to be CC'd on this]
On Sep 2, 2011, at 9:47 AM, Anton Yartsev wrote:
> 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
===================================================================
--- 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.
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.
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?
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
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.
Index: lib/Headers/mm_malloc.h
===================================================================
--- lib/Headers/mm_malloc.h (revision 138994)
+++ lib/Headers/mm_malloc.h (working copy)
@@ -25,6 +25,9 @@
#define __MM_MALLOC_H
#include <stdlib.h>
+#ifdef __cplusplus
+#include <new>
+#endif
This should also check __STRICT_ALIGNED before including <new>, because we don't want <new> cluttering up our header unless forced into it.
- Doug
More information about the cfe-dev
mailing list