[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D16738: Fix invalid casts in <functional>.
From:       Eric Fiselier via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2016-01-31 3:57:53
Message-ID: c6b43a194bc422ab5038e3f8ee42d168 () localhost ! localdomain
[Download RAW message or body]

EricWF added a comment.

> This also could be fixed in a different way by replacing C-style

> casts with reinterpret_cast<>, which, from my reading of the

> standard, is allowed in this context.


I agree that using `void*` to represent raw memory is the better approach than \
`reinterpret_cast<>()`. However I'm concerned that changing the signature (and \
mangling) of `virtual void __clone(...)` could cause ABI problems. I *think* this \
should be "safe" because the VTable's mangled name doesn't change. but if I'm wrong \
we must use `reinterpret_cast<>` for calls to `__clone(...)`.

The parts of the patch that don't affect `__clone(...)` LGTM. You can commit them \
separably if you want.

> That would not help with CFI

> though, which still flags such casts as invalid (yes, it is stricter that the \
> standard).


I'm sure there are alternative ways to make CFI shut up. Perhaps we could do the \
`Buffer* -> Base*` conversion inside a blacklisted function (akin to std::launder)? \
It would also be nice to have "`__attribute__((__no_sanitize__("cfi")))`.


================
Comment at: include/functional:1443
@@ -1442,3 +1442,3 @@
     virtual __base* __clone() const = 0;
-    virtual void __clone(__base*) const = 0;
+    virtual void __clone(void*) const = 0;
     virtual void destroy() _NOEXCEPT = 0;
----------------
Doesn't this break the ABI because it changes the mangling of a virtual function? I \
understand the vtable should remain layout compatible but I suspect the mangling \
change is an issue.

================
Comment at: include/functional:1739
@@ -1738,2 +1738,3 @@
         {
+            ::new (&__buf_) _FF(_VSTD::move(__f));
             __f_ = (__base*)&__buf_;
----------------
`__f_ = ::new((void*)&__buf_) _FF(_VSTD::move(__f));`


Repository:
  rL LLVM

http://reviews.llvm.org/D16738



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]