[cfe-dev] aligned 'new'
Anton Yartsev
anton.yartsev at gmail.com
Mon Oct 24 21:17:28 PDT 2011
Attached are the updated files.
Please review!
> 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
>
--
Anton
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new.h
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111025/03f16849/attachment.h>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new_test.cpp
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111025/03f16849/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aligned_new_v1.patch
Type: text/x-diff
Size: 28281 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111025/03f16849/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: HelperLibs.zip
Type: application/x-zip-compressed
Size: 3913 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111025/03f16849/attachment.bin>
More information about the cfe-dev
mailing list