<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 19, 2015 at 12:29 PM, Larisse Voufo <span dir="ltr"><<a href="mailto:lvoufo@google.com" target="_blank">lvoufo@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><p dir="ltr"><br>
On Feb 17, 2015 5:07 PM, "Larisse Voufo" <<a href="mailto:lvoufo@google.com" target="_blank">lvoufo@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Sun, Feb 15, 2015 at 6:10 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> On Fri, Feb 13, 2015 at 9:42 PM, Larisse Voufo <<a href="mailto:lvoufo@google.com" target="_blank">lvoufo@google.com</a>> wrote:<br>
>>><br>
>>> Author: lvoufo<br>
>>> Date: Fri Feb 13 23:42:57 2015<br>
>>> New Revision: 229241<br>
>>><br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=229241&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=229241&view=rev</a><br>
>>> Log:<br>
>>> Revise the implementation logic of sized deallocation: Do not automatically generate weak definitions of the sized operator delete (in terms of unsized operator delete). Instead, provide the funcitonality via a new compiler flag, -fdef-sized-delete.<br>
>>> The current implementation causes link-time ODR violations when the delete symbols are exported into the dynamic table.<br>
>>><br>
>>> Modified:<br>
>>> cfe/trunk/include/clang/Basic/LangOptions.def<br>
>>> cfe/trunk/include/clang/Driver/Options.td<br>
>>> cfe/trunk/lib/CodeGen/CodeGenFunction.cpp<br>
>>> cfe/trunk/lib/CodeGen/CodeGenModule.cpp<br>
>>> cfe/trunk/lib/Driver/Tools.cpp<br>
>>> cfe/trunk/lib/Frontend/CompilerInvocation.cpp<br>
>>> cfe/trunk/test/CodeGenCXX/cxx1y-sized-deallocation.cpp<br>
>>> cfe/trunk/test/CodeGenCXX/implicit-allocation-functions.cpp<br>
>>><br>
>>> Modified: cfe/trunk/include/clang/Basic/LangOptions.def<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/include/clang/Basic/LangOptions.def (original)<br>
>>> +++ cfe/trunk/include/clang/Basic/LangOptions.def Fri Feb 13 23:42:57 2015<br>
>>> @@ -162,6 +162,7 @@ LANGOPT(CUDAIsDevice , 1, 0, "Compi<br>
>>><br>
>>> LANGOPT(AssumeSaneOperatorNew , 1, 1, "implicit __attribute__((malloc)) for C++'s new operators")<br>
>>> LANGOPT(SizedDeallocation , 1, 0, "enable sized deallocation functions")<br>
>>> +LANGOPT(DefaultSizedDelete , 1, 0, "Generate weak definitions of sized delete")<br>
>><br>
>><br>
>> Please start this string with a lowercase letter. How about DefineSizedDeallocation, to match the existing SizedDeallocation LangOpt? ("Define" seems like a better word than "Default" here in any case.)<br>
>> <br>
>>><br>
>>> BENIGN_LANGOPT(ElideConstructors , 1, 1, "C++ copy constructor elision")<br>
>>> BENIGN_LANGOPT(DumpRecordLayouts , 1, 0, "dumping the layout of IRgen'd records")<br>
>>> BENIGN_LANGOPT(DumpRecordLayoutsSimple , 1, 0, "dumping the layout of IRgen'd records in a simple form")<br>
>>><br>
>>> Modified: cfe/trunk/include/clang/Driver/Options.td<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/include/clang/Driver/Options.td (original)<br>
>>> +++ cfe/trunk/include/clang/Driver/Options.td Fri Feb 13 23:42:57 2015<br>
>>> @@ -394,6 +394,8 @@ def fasm_blocks : Flag<["-"], "fasm-bloc<br>
>>> def fno_asm_blocks : Flag<["-"], "fno-asm-blocks">, Group<f_Group>;<br>
>>><br>
>>> def fassume_sane_operator_new : Flag<["-"], "fassume-sane-operator-new">, Group<f_Group>;<br>
>>> +def fdef_sized_delete: Flag<["-"], "fdef-sized-delete">, Group<f_Group>,<br>
>>> + HelpText<"Allow compiler-generated definition of sized deallocation function">, Flags<[CC1Option]>;<br>
>><br>
>><br>
>> Likewise here, how about -fdefine-sized-deallocation, to match the existing -fsized-deallocation flag?<br>
><br>
><br>
> Thanks, Richard. Please see r229597 for the above comments.<br>
> <br>
>><br>
>><br>
>>> def fastcp : Flag<["-"], "fastcp">, Group<f_Group>;<br>
>>> def fastf : Flag<["-"], "fastf">, Group<f_Group>;<br>
>>> def fast : Flag<["-"], "fast">, Group<f_Group>;<br>
>>><br>
>>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)<br>
>>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Fri Feb 13 23:42:57 2015<br>
>>> @@ -891,8 +891,11 @@ void CodeGenFunction::GenerateCode(Globa<br>
>>> } else if (FunctionDecl *UnsizedDealloc =<br>
>>> FD->getCorrespondingUnsizedGlobalDeallocationFunction()) {<br>
>>> // Global sized deallocation functions get an implicit weak definition if<br>
>>> - // they don't have an explicit definition.<br>
>>> + // they don't have an explicit definition, if allowed.<br>
>>> + assert(getLangOpts().DefaultSizedDelete &&<br>
>>> + "Can't emit unallowed definition.");<br>
>>> EmitSizedDeallocationFunction(*this, UnsizedDealloc);<br>
>>> +<br>
>>> } else<br>
>>> llvm_unreachable("no definition for emitted function");<br>
>>><br>
>>><br>
>>> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)<br>
>>> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Fri Feb 13 23:42:57 2015<br>
>>> @@ -1615,13 +1615,13 @@ CodeGenModule::GetOrCreateLLVMFunction(S<br>
>>> // don't need it anymore).<br>
>>> addDeferredDeclToEmit(F, DDI->second);<br>
>>> DeferredDecls.erase(DDI);<br>
>>> -<br>
>>> +<br>
>>> // Otherwise, if this is a sized deallocation function, emit a weak<br>
>>> - // definition<br>
>>> - // for it at the end of the translation unit.<br>
>>> + // definition for it at the end of the translation unit.<br>
>>> } else if (D && cast<FunctionDecl>(D)<br>
>>> ->getCorrespondingUnsizedGlobalDeallocationFunction()) {<br>
>>> - addDeferredDeclToEmit(F, GD);<br>
>>> + if (getLangOpts().DefaultSizedDelete)<br>
>>> + addDeferredDeclToEmit(F, GD);<br>
>>><br>
>>> // Otherwise, there are cases we have to worry about where we're<br>
>>> // using a declaration for which we must emit a definition but where<br>
>>><br>
>>> Modified: cfe/trunk/lib/Driver/Tools.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/lib/Driver/Tools.cpp (original)<br>
>>> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Feb 13 23:42:57 2015<br>
>>> @@ -4243,6 +4243,11 @@ void Clang::ConstructJob(Compilation &C,<br>
>>> if (!Args.hasFlag(options::OPT_fassume_sane_operator_new,<br>
>>> options::OPT_fno_assume_sane_operator_new))<br>
>>> CmdArgs.push_back("-fno-assume-sane-operator-new");<br>
>>> +<br>
>>> + // -def-sized-delete: default implementation of sized delete as a<br>
>>> + // weak definition.<br>
>>> + if (Args.hasArg(options::OPT_fdef_sized_delete))<br>
>>> + CmdArgs.push_back("-fdef-sized-delete");<br>
>>><br>
>>> // -fconstant-cfstrings is default, and may be subject to argument translation<br>
>>> // on Darwin.<br>
>>><br>
>>> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)<br>
>>> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Feb 13 23:42:57 2015<br>
>>> @@ -1525,6 +1525,8 @@ static void ParseLangArgs(LangOptions &O<br>
>>> Opts.NoMathBuiltin = Args.hasArg(OPT_fno_math_builtin);<br>
>>> Opts.AssumeSaneOperatorNew = !Args.hasArg(OPT_fno_assume_sane_operator_new);<br>
>>> Opts.SizedDeallocation |= Args.hasArg(OPT_fsized_deallocation);<br>
>>> + Opts.DefaultSizedDelete = Opts.SizedDeallocation &&<br>
>>> + Args.hasArg(OPT_fdef_sized_delete);<br>
>>> Opts.HeinousExtensions = Args.hasArg(OPT_fheinous_gnu_extensions);<br>
>>> Opts.AccessControl = !Args.hasArg(OPT_fno_access_control);<br>
>>> Opts.ElideConstructors = !Args.hasArg(OPT_fno_elide_constructors);<br>
>>><br>
>>> Modified: cfe/trunk/test/CodeGenCXX/cxx1y-sized-deallocation.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/cxx1y-sized-deallocation.cpp?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/cxx1y-sized-deallocation.cpp?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/test/CodeGenCXX/cxx1y-sized-deallocation.cpp (original)<br>
>>> +++ cfe/trunk/test/CodeGenCXX/cxx1y-sized-deallocation.cpp Fri Feb 13 23:42:57 2015<br>
>>> @@ -1,5 +1,7 @@<br>
>>> -// RUN: %clang_cc1 -std=c++1y %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s<br>
>>> -// RUN: %clang_cc1 -std=c++11 -fsized-deallocation %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s<br>
>>> +// RUN: %clang_cc1 -std=c++1y %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECKUND<br>
>>> +// RUN: %clang_cc1 -std=c++1y %s -emit-llvm -triple x86_64-linux-gnu -fdef-sized-delete -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECKDEF<br>
>>> +// RUN: %clang_cc1 -std=c++11 -fsized-deallocation %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECKUND<br>
>>> +// RUN: %clang_cc1 -std=c++11 -fsized-deallocation -fdef-sized-delete %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECKDEF<br>
>>> // RUN: %clang_cc1 -std=c++11 %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefix=CHECK-UNSIZED<br>
>>><br>
>>> // CHECK-UNSIZED-NOT: _ZdlPvm<br>
>>> @@ -50,8 +52,9 @@ D::D() {}<br>
>>> // CHECK: call void @_ZdlPvm(i8* %{{[^ ]*}}, i64 4)<br>
>>> // CHECK: call void @_ZdaPv(i8* %{{[^ ]*}})<br>
>>><br>
>>> -// CHECK-LABEL: define linkonce void @_ZdlPvm(i8*<br>
>>> -// CHECK: call void @_ZdlPv(i8* %0)<br>
>>> +// CHECKDEF-LABEL: define linkonce void @_ZdlPvm(i8*<br>
>>> +// CHECKDEF: call void @_ZdlPv(i8* %0)<br>
>>> +// CHECKUND-LABEL: declare void @_ZdlPvm(i8*<br>
>>><br>
>>> // CHECK-LABEL: define weak_odr void @_Z3delI1BEvv()<br>
>>> // CHECK: call void @_ZdlPvm(i8* %{{[^ ]*}}, i64 4)<br>
>>> @@ -71,8 +74,9 @@ D::D() {}<br>
>>> // CHECK: add i64 %{{[^ ]*}}, 8<br>
>>> // CHECK: call void @_ZdaPvm(i8* %{{[^ ]*}}, i64 %{{[^ ]*}})<br>
>>><br>
>>> -// CHECK-LABEL: define linkonce void @_ZdaPvm(i8*<br>
>>> -// CHECK: call void @_ZdaPv(i8* %0)<br>
>>> +// CHECKDEF-LABEL: define linkonce void @_ZdaPvm(i8*<br>
>>> +// CHECKDEF: call void @_ZdaPv(i8* %0)<br>
>>> +// CHECKUND-LABEL: declare void @_ZdaPvm(i8*<br>
>>><br>
>>> // CHECK-LABEL: define weak_odr void @_Z3delI1DEvv()<br>
>>> // CHECK: call void @_ZdlPvm(i8* %{{[^ ]*}}, i64 8)<br>
>>><br>
>>> Modified: cfe/trunk/test/CodeGenCXX/implicit-allocation-functions.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/implicit-allocation-functions.cpp?rev=229241&r1=229240&r2=229241&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/implicit-allocation-functions.cpp?rev=229241&r1=229240&r2=229241&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- cfe/trunk/test/CodeGenCXX/implicit-allocation-functions.cpp (original)<br>
>>> +++ cfe/trunk/test/CodeGenCXX/implicit-allocation-functions.cpp Fri Feb 13 23:42:57 2015<br>
>>> @@ -1,7 +1,9 @@<br>
>>> // RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++11 %s 2>&1 | FileCheck %s -check-prefix=CHECKDEF -check-prefix=CHECK11<br>
>>> // RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++11 -fvisibility hidden %s 2>&1 | FileCheck %s -check-prefix=CHECKHID -check-prefix=CHECK11<br>
>>> -// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++14 %s 2>&1 | FileCheck %s -check-prefix=CHECKDEF -check-prefix=CHECK14<br>
>>> -// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++14 -fvisibility hidden %s 2>&1 | FileCheck %s -check-prefix=CHECKHID -check-prefix=CHECK14<br>
>>> +// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++14 %s 2>&1 | FileCheck %s -check-prefix=CHECKDEF -check-prefix=CHECK14 -check-prefix=CHECK14UND<br>
>>> +// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++14 -fvisibility hidden %s 2>&1 | FileCheck %s -check-prefix=CHECKHID -check-prefix=CHECK14 -check-prefix=CHECK14UND<br>
>>> +// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++14 -fdef-sized-delete %s 2>&1 | FileCheck %s -check-prefix=CHECKDEF -check-prefix=CHECK14 -check-prefix=CHECK14DEF<br>
>>> +// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown -o - -std=c++14 -fdef-sized-delete -fvisibility hidden %s 2>&1 | FileCheck %s -check-prefix=CHECKHID -check-prefix=CHECK14 -check-prefix=CHECK14DEF<br>
>>><br>
>>> // PR22419: Implicit sized deallocation functions always have default visibility.<br>
>>> // Generalized to all implicit allocation functions.<br>
>>> @@ -27,8 +29,9 @@ void foo(A* is) {<br>
>>> // CHECK11-DAG: declare void @_ZdlPv(i8*)<br>
>>><br>
>>> // CHECK14-DAG: declare noalias i8* @_Znwm(i64)<br>
>>> -// CHECK14-DAG: define linkonce void @_ZdlPvm(i8*, i64)<br>
>>> -// CHECK14-DAG: declare void @_ZdlPv(i8*)<br>
>>> +// CHECK14UND-DAG: declare void @_ZdlPvm(i8*, i64)<br>
>>> +// CHECK14DEF-DAG: define linkonce void @_ZdlPvm(i8*, i64)<br>
>>> +// CHECK14DEF-DAG: declare void @_ZdlPv(i8*)<br>
>>><br>
>>> // CHECK14-DAG: %struct.B = type { i8 }<br>
>>> struct B { ~B() { }};<br>
>>> @@ -50,5 +53,6 @@ void f(B *p) {<br>
>>> // CHECK11-DAG: declare void @_ZdaPv(i8*)<br>
>>><br>
>>> // CHECK14-DAG: declare noalias i8* @_Znam(i64)<br>
>>> -// CHECK14-DAG: define linkonce void @_ZdaPvm(i8*, i64)<br>
>>> -// CHECK14-DAG: declare void @_ZdaPv(i8*)<br>
>>> +// CHECK14UND-DAG: declare void @_ZdaPvm(i8*, i64)<br>
>>> +// CHECK14DEF-DAG: define linkonce void @_ZdaPvm(i8*, i64)<br>
>>> +// CHECK14DEF-DAG: declare void @_ZdaPv(i8*)<br>
>><br>
>><br>
>> Please can you update cxx_status.html and the Clang 3.7 release notes to note that we now require one of (a) libc++ 3.7 or later, (b) libstdc++ 5 or later, (c) -fdefine-sized-deallocation, or (d) -fno-sized-deallocation to build code in C++14 mode? </p>
</div></div><p dir="ltr">Hi Richard -- did you mean -fno-sized-deallocation here?</p></blockquote><div>I did, but it looks like we don't expose these flags in the driver yet.</div><div><br></div><div>We've had some more discussion of this, and have a new proposal for a way forward, as follows. Clang should provide three modes for sized deallocation:</div><div><br></div><div>-fsized-deallocation=never -- never call a sized deallocation function</div><div>-fsized-deallocation=check -- use a sized deallocation function if it's available</div><div>-fsized-deallocation=always -- call a sized deallocation, assuming that someone defines it</div><div><br></div><div>(names here may need more work).</div><div><br></div><div>If none of these is explicitly specified, C++14 onwards uses "check" and all other modes use "never".</div><div><br></div><div>The "check" mode would generate code for a delete operator that checks whether a sized deallocation function is available, and calls it if it is, and calls the unsized deallocation function otherwise. We can implement this by emitting an extern_weak declaration of the sized deallocation, with each call site checking whether the declaration is null, and calling either it or the unsized deallocation, or by using the existing code to generate a weak definition if extern_weak linkage isn't available on our target.</div></div></div></div>