[cfe-dev] aligned 'new'

[Anton Yartsev]

On 20.09.2011 22:37, Anton Yartsev wrote:
> 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
>
ping

-- 
Anton