Page MenuHomePhabricator

[libc++] Remove UB in std::list
Needs ReviewPublic

Authored by akhuang on Apr 23 2021, 4:22 PM.

Details

Reviewers
ldionne
EricWF
Quuxplusone
Mordante
Group Reviewers
Restricted Project
Summary

The constructor of __list_node is never called, because it has a
member (__value_) that can't be constructed when we construct __list_node.

Putting __value_ in a union solves the issue for C++11 and above, but C++03
doesn't allow putting classes with a non-trivial copy constructor in a union.

This patch stores __value_ as aligned storage bytes when using c++03 and puts
reinterpret_casts everywhere it's used.

Continuation of https://reviews.llvm.org/D99624.

Diff Detail

Event Timeline

akhuang requested review of this revision.Apr 23 2021, 4:22 PM
akhuang created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2021, 4:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
akhuang updated this revision to Diff 340196.Apr 23 2021, 4:26 PM

remove commented out code

akhuang updated this revision to Diff 340198.Apr 23 2021, 4:28 PM

remove commented code

This looks somewhat similar to D99624. Does it attempt to address the same issue? @ldionne you might want to have a look at this.

Quuxplusone added a subscriber: Quuxplusone.

@Mordante: yes it does. :)
@akhuang: My understanding is that removing-the-UB is needed (or at least desired) for the benefit of some particular sanitizer; could you add info to the review summary (and commit message) about that?
Also, are we really interested in getting that benefit for C++03 code? Arguably it would be better to preserve the existing behavior for C++03 and fix the behavior only for C++>=11. Is it physically possible to go that route? and if so, does that eliminate a lot of the ifdefs in this PR? (I'm thinking we could just have one ifdef around the union itself, and then the rest of the code wouldn't need ifdefs at all.) The behavior would remain UB, but unchanged, in C++03; and we could say "if you want to use such-and-such sanitizer, please compile as C++>=11." That won't be a hardship for the user, will it?

@Mordante: yes it does. :)
@akhuang: My understanding is that removing-the-UB is needed (or at least desired) for the benefit of some particular sanitizer; could you add info to the review summary (and commit message) about that?
Also, are we really interested in getting that benefit for C++03 code? Arguably it would be better to preserve the existing behavior for C++03 and fix the behavior only for C++>=11. Is it physically possible to go that route? and if so, does that eliminate a lot of the ifdefs in this PR? (I'm thinking we could just have one ifdef around the union itself, and then the rest of the code wouldn't need ifdefs at all.) The behavior would remain UB, but unchanged, in C++03; and we could say "if you want to use such-and-such sanitizer, please compile as C++>=11." That won't be a hardship for the user, will it?

So the motivation for this is for a debug info optimization that homes debug info for classes to their constructors, and it relies on constructors being called (and we'd like to enable that mode by default in llvm). We don't necessarily care whether there's UB or not, but in this situation we wouldn't have debug info for these types.

I think making it only work for only C++>=11 could be fine, though.

For the debug info issue, we can also just add an attribute to these types (https://reviews.llvm.org/D98750); I think @ldionne preferred solving this by fixing the UB directly though.

akhuang updated this revision to Diff 340581.Apr 26 2021, 10:33 AM

Remove fix for c++03.

LGTM at this point. So I guess the next question (besides how it looks to @ldionne!) is, should you do it for forward_list etc. in this same PR? Do you know already the complete list of types that need this kind of patch done for them? If so, I would think it's appropriate to do them all together in this PR.

libcxx/include/list
274

Nit: Indent 4 spaces to match the indentation everywhere else in this file. (I blame clang-format. ;))

282–285

Any reason not to =default this constructor? (And I'd make it explicit, since there's no reason not to, and it might ease overload resolution just a tiny bit. In my codebases, the explicit keyword basically means "Look out, here comes a constructor!")

akhuang updated this revision to Diff 341014.Apr 27 2021, 3:58 PM

Apply union fix to forward_list_node and hash_node

Seems like it's a little less straightforward for some of the other types (__tree_node, __value_type and __hash_value_type), especially since they explicitly delete the constructors/destructors and I don't have a great understanding of why. Should probably get @EricWF's opinion on this?

I'm still somewhat leaning toward just adding an attribute to those types to solve the debug info issue (https://reviews.llvm.org/D98750)

libcxx/include/list
282–285

apparently using =default complains because __value_ doesn't have a default constructor

Has this been tested with various sanitizers on? Does it give warnings about weird object lifetimes?

libcxx/include/list
274

I'm a bit worried that there are combinations of types and compiler flags that will align a subobject differently when it's in a union than when it's not.
I need to investigate further.

1125

Maybe we can sink these repeated lines of code into a function?

Seems like it's a little less straightforward for some of the other types (__tree_node, __value_type and __hash_value_type), especially since they explicitly delete the constructors/destructors and I don't have a great understanding of why. Should probably get @EricWF's opinion on this?

I'm still somewhat leaning toward just adding an attribute to those types to solve the debug info issue (https://reviews.llvm.org/D98750)

I agree that the attribute is a non-complicated and good way to address those types.

Has this been tested with various sanitizers on? Does it give warnings about weird object lifetimes?

Tried running tests with sanitizers (Address, Memory, Undefined) - doesn't look like there are any new failures.

akhuang updated this revision to Diff 341749.Apr 29 2021, 7:41 PM

Move some of the #ifndefs in forward_list into a function

akhuang updated this revision to Diff 342788.May 4 2021, 10:08 AM

Fix #ifdef for function