Oilpan: tighten DISALLOW_ALLOCATION so it does not allow inline

Oilpan: tighten DISALLOW_ALLOCATION so it does not allow inline
allocation by user-defined placement new operators.

Removing the placement new operators will lead to redefinition errors
if a class is marked as DISALLOW_ALLOCATION or STACK_ALLOCATED
and at the same time defines placement new operators.

R=oilpan-reviews@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169642

[Mads Ager (chromium), 2014-03-20 07:28:04]

https://codereview.chromium.org/199733009/diff/1/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/199733009/diff/1/Source/heap/Heap.h#newcode1069
Source/heap/Heap.h:1069: DISALLOW_ALLOCATION()
This doesn't work. I'll have to add them all here with annotations. :-)

[zerny-chromium, 2014-03-20 07:30:02]

https://codereview.chromium.org/199733009/diff/1/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/199733009/diff/1/Source/heap/Heap.h#newcode1069
Source/heap/Heap.h:1069: DISALLOW_ALLOCATION()
I'm not sure if this will work now that there is a "private:" between the
annotation and the "operator new(size_t)" that was previously being annotated.
This also makes it unclear what method is being annotated (if any) because we
need to unfold the DISALLOW_ALLOCATION macro.

[zerny-chromium, 2014-03-20 07:32:58]

> This doesn't work. I'll have to add them all here with annotations. :-)

Yeah. Is is unfortunate. I'd really rather have had the class declaration
annotated but I can't find a way to do that with our current placement of
STACK_ALLOCATED() inside the declaration body.

Alternatively, we could add a no-op method "void stack_allocated();" and
annotate that? Would also be nicer in the plugin code because right now I will
need to iterate all operator new in search of an annotation.

[Mads Ager (chromium), 2014-03-20 07:33:13]

On 2014/03/20 07:30:02, zerny-chromium wrote:
> https://codereview.chromium.org/199733009/diff/1/Source/heap/Heap.h
> File Source/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/199733009/diff/1/Source/heap/Heap.h#newcode1069
> Source/heap/Heap.h:1069: DISALLOW_ALLOCATION()
> I'm not sure if this will work now that there is a "private:" between the
> annotation and the "operator new(size_t)" that was previously being annotated.
> This also makes it unclear what method is being annotated (if any) because we
> need to unfold the DISALLOW_ALLOCATION macro.

Update.

[Mads Ager (chromium), 2014-03-20 07:35:28]

On 2014/03/20 07:32:58, zerny-chromium wrote:
> > This doesn't work. I'll have to add them all here with annotations. :-)
> 
> Yeah. Is is unfortunate. I'd really rather have had the class declaration
> annotated but I can't find a way to do that with our current placement of
> STACK_ALLOCATED() inside the declaration body.
> 
> Alternatively, we could add a no-op method "void stack_allocated();" and
> annotate that? Would also be nicer in the plugin code because right now I will
> need to iterate all operator new in search of an annotation.

For now I have added the annotation to one of them. The other argument would be
that it would be nice to not block any names from being used and in that sense
annotating one of the new operators seems reasonable.

[zerny-chromium, 2014-03-20 07:37:09]

On 2014/03/20 07:35:28, Mads Ager (chromium) wrote:
> On 2014/03/20 07:32:58, zerny-chromium wrote:
> > > This doesn't work. I'll have to add them all here with annotations. :-)
> > 
> > Yeah. Is is unfortunate. I'd really rather have had the class declaration
> > annotated but I can't find a way to do that with our current placement of
> > STACK_ALLOCATED() inside the declaration body.
> > 
> > Alternatively, we could add a no-op method "void stack_allocated();" and
> > annotate that? Would also be nicer in the plugin code because right now I
will
> > need to iterate all operator new in search of an annotation.
> 
> For now I have added the annotation to one of them. The other argument would
be
> that it would be nice to not block any names from being used and in that sense
> annotating one of the new operators seems reasonable.

Agreed. I'll iterate all "operator new" when searching for the annotation and
memoize the result. That should be fine.

lgtm!

[Mads Ager (chromium), 2014-03-20 07:38:30]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/199733009/10002

[commit-bot: I haz the power, 2014-03-20 07:38:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:38:35]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Vyacheslav Egorov (Chromium), 2014-03-20 07:41:54]

lgtm

[Mads Ager (chromium), 2014-03-20 07:45:49]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:45:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/199733009/10002

[commit-bot: I haz the power, 2014-03-20 07:46:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:46:01]

Failed to apply patch for Source/heap/Heap.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/heap/Heap.h
  Hunk #2 FAILED at 1063.
  1 out of 2 hunks FAILED -- saving rejects to file Source/heap/Heap.h.rej

Patch:       Source/heap/Heap.h
Index: Source/heap/Heap.h
diff --git a/Source/heap/Heap.h b/Source/heap/Heap.h
index
5904f48118862b05c1ee4c1a71afef1eb368dbe4..94d7803ffbcc8cabbc752246cf949ab2afbacaa1
100644
--- a/Source/heap/Heap.h
+++ b/Source/heap/Heap.h
@@ -1025,20 +1025,36 @@ T* adoptRefCountedGarbageCollected(T* ptr)
 }
 
 #if COMPILER_SUPPORTS(CXX_DELETED_FUNCTIONS)
-#define DISALLOW_ALLOCATION() \
-    private: \
+#define DISALLOW_ALLOCATION()                                   \
+    private:                                                    \
+        void* operator new(size_t) = delete;                    \
+        void* operator new(size_t, NotNullTag, void*) = delete; \
+        void* operator new (size_t, void*) = delete;
+
+#define ALLOW_ONLY_INLINE_ALLOCATION()                                         
    \
+    public:                                                                    
    \
+        void* operator new(size_t, NotNullTag, void* location) { return
location; } \
+        void* operator new(size_t, void* location) { return location; }        
    \
+    private:                                                                   
    \
         void* operator new(size_t) = delete;
+
 #else
-#define DISALLOW_ALLOCATION() \
-    private: \
-        void* operator new(size_t);
-#endif
+
+#define DISALLOW_ALLOCATION()                          \
+    private:                                           \
+        void* operator new(size_t);                    \
+        void* operator new(size_t, NotNullTag, void*); \
+        void* operator new(size_t, void*)
 
 #define ALLOW_ONLY_INLINE_ALLOCATION()                                         
    \
     public:                                                                    
    \
         void* operator new(size_t, NotNullTag, void* location) { return
location; } \
         void* operator new(size_t, void* location) { return location; }        
    \
-    DISALLOW_ALLOCATION()
+    private:                                                                   
    \
+        void operator new(size_t);
+
+#endif
+
 
 // These macros insert annotations that the Blink GC plugin for clang uses for
 // verification. STACK_ALLOCATED is used to declare that objects of this type
@@ -1047,10 +1063,14 @@ T* adoptRefCountedGarbageCollected(T* ptr)
 // GC_PLUGIN_IGNORE a bug-number should be provided as an argument where the
 // bug describes what needs to happen to remove the GC_PLUGIN_IGNORE again.
 #if COMPILER(CLANG)
-#define STACK_ALLOCATED()                               \
-    private:                                            \
-        __attribute__((annotate("blink_stack_allocated")))    \
-        void* operator new(size_t) = delete;
+#define STACK_ALLOCATED()                                       \
+    private:                                                    \
+        __attribute__((annotate("blink_stack_allocated")))      \
+        void* operator new(size_t) = delete;                    \
+        void* operator new(size_t, NotNullTag, void*) = delete; \
+        void* operator new (size_t, void*) = delete;
+
+
 #define GC_PLUGIN_IGNORE(bug)                           \
     __attribute__((annotate("blink_gc_plugin_ignore")))
 #else

[Erik Corry, 2014-03-20 07:46:41]

LGTM with comments.

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1032: void* operator new (size_t, void*) = delete;
Spurious space here

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1054: void operator new(size_t);
Missing asterisk here

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1071: void* operator new (size_t, void*) = delete;
Spurious space here.

[Mads Ager (chromium), 2014-03-20 07:54:22]

Thanks Erik. Those were silly. :-)

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1032: void* operator new (size_t, void*) = delete;
On 2014/03/20 07:46:42, Erik Corry wrote:
> Spurious space here

Done.

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1054: void operator new(size_t);
On 2014/03/20 07:46:42, Erik Corry wrote:
> Missing asterisk here

Done.

https://codereview.chromium.org/199733009/diff/10002/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1071: void* operator new (size_t, void*) = delete;
On 2014/03/20 07:46:42, Erik Corry wrote:
> Spurious space here.

Done.

[Mads Ager (chromium), 2014-03-20 07:54:27]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:54:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/199733009/30001

[commit-bot: I haz the power, 2014-03-20 07:55:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:55:31]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[Mads Ager (chromium), 2014-03-20 07:56:16]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-20 07:56:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/199733009/30001

[commit-bot: I haz the power, 2014-03-20 09:06:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 09:06:38]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Mads Ager (chromium), 2014-03-20 09:55:55]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-20 09:56:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/199733009/30001

[commit-bot: I haz the power, 2014-03-20 11:00:50]

Change committed as 169642