<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 23, 2014 at 11:39 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.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">On Wed, Apr 23, 2014 at 2:16 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>

> On Wed, Apr 23, 2014 at 9:26 AM, Abramo Bagnara <<a href="mailto:abramo.bagnara@bugseng.com">abramo.bagnara@bugseng.com</a>><br>
> wrote:<br>
>><br>
>> In r205915 you've added to ThreadSafetyTIL.h a mechanism to avoid<br>
>> explicit delete for SExpr, but this break compilation with REQUIRES_EH=1<br>
>> as shown below:<br>
>><br>
>> $ make VERBOSE=1 REQUIRES_EH=1<br>
>> llvm[0]: Compiling ThreadSafety.cpp for Debug+Asserts build<br>
>> if  g++ -Illvm-r206978-build/include<br>
>> -Illvm-r206978-build/tools/clang/lib/Analysis -Illvm-r206978/include<br>
>> -Illvm-r206978/tools/clang/lib/Analysis  -D_DEBUG -D_GNU_SOURCE<br>
>> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS<br>
>> -Illvm-r206978/tools/clang/lib/Analysis/../../include<br>
>> -Illvm-r206978-build/tools/clang/lib/Analysis/../../include -g<br>
>> -std=c++11 -fvisibility-inlines-hidden -fPIC -Woverloaded-virtual<br>
>> -ffunction-sections -fdata-sections -Wcast-qual -fno-strict-aliasing<br>
>> -pedantic -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings<br>
>> -I/home/abramo/eclair_cplusplus/deps/traps/include<br>
>> -Wno-maybe-uninitialized -Wno-missing-field-initializers -c -MMD -MP -MF<br>
>><br>
>> "llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.d.tmp"<br>
>> -MT<br>
>> "llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.o"<br>
>> -MT<br>
>> "llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.d"<br>
>> llvm-r206978/tools/clang/lib/Analysis/ThreadSafety.cpp -o<br>
>> llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.o ;<br>
>> \<br>
>>                 then /bin/mv -f<br>
>><br>
>> "llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.d.tmp"<br>
>><br>
>> "llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.d";<br>
>> else /bin/rm<br>
>><br>
>> "llvm-r206978-build/tools/clang/lib/Analysis/Debug+Asserts/ThreadSafety.d.tmp";<br>
>> exit 1; fi<br>
>> In file included from<br>
>> llvm-r206978/tools/clang/lib/Analysis/ThreadSafety.cpp:25:0:<br>
>><br>
>> llvm-r206978/tools/clang/lib/Analysis/../../include/clang/Analysis/Analyses/ThreadSafetyTIL.h:<br>
>> In member function ‘clang::threadSafety::til::SExpr*<br>
>><br>
>> clang::threadSafety::til::CopyReducer::reduceUndefined(clang::threadSafety::til::Undefined&)’:<br>
>><br>
>> llvm-r206978/tools/clang/lib/Analysis/../../include/clang/Analysis/Analyses/ThreadSafetyTIL.h:120:8:<br>
>> error: ‘static void clang::threadSafety::til::SExpr::operator<br>
>> delete(void*)’ is private<br>
>>    void operator delete(void *) LLVM_DELETED_FUNCTION;<br>
>>         ^<br>
>> In file included from<br>
>> llvm-r206978/tools/clang/lib/Analysis/ThreadSafety.cpp:26:0:<br>
>><br>
>> llvm-r206978/tools/clang/lib/Analysis/../../include/clang/Analysis/Analyses/ThreadSafetyTraverse.h:127:38:<br>
>> error: within this context<br>
>>      return new (Arena) Undefined(Orig);<br>
>>                                       ^<br>
>> ... and many others.<br>
>><br>
>> I don't think this is expected: what do you think about to remove the<br>
>> private deleted operator delete? Are there better way to fix that?<br>
><br>
><br>
> This looks like a GCC bug; it shouldn't be checking accessibility on this<br>
> 'operator delete' here, as far as I can see. The simplest fix is to make the<br>
> 'operator delete' public -- it's already marked LLVM_DELETED_FUNCTION, and<br>
> that's sufficient to stop these objects getting accidentally deleted.<br>
<br>
</div></div>Except on systems like MSVC 2012, where LLVM_DELETED_FUNCTION is a<br>
noop (so it's meant to be a declaration without a definition and<br>
trigger link errors). Though, given the alternative (non-compiling<br>
build for a broken gcc), I think it's a reasonable approach.</blockquote><div><br></div><div>It's still sufficient to stop these objects getting accidentally deleted if even one person/buildbot builds with a compiler that supports =delete. This is the whole point of LLVM_DELETED_FUNCTION, and we already accept that some errors might make it through to a buildbot in all the other places where we use it.</div>
</div></div></div>