Page MenuHomePhabricator

Apply [[standalone_debug]] to some types in the STL.
Needs ReviewPublic

Authored by akhuang on Mar 16 2021, 4:30 PM.

Details

Reviewers
EricWF
ldionne
rnk
dblaikie
Quuxplusone
Group Reviewers
Restricted Project
Summary

Add this attribute to some types to ensure that they have
debug info.
The debug info for these classes are required for debuggers to display
some STL types. With constructor homing (a new debug info optimization)
their debug info isn't emitted because their constructors are never
called.

The list of types with the attribute added are hash_value_type,
value_type, tree_node_base, tree_node, hash_node, list_node,
and __forward_list_node.

Diff Detail

Event Timeline

akhuang requested review of this revision.Mar 16 2021, 4:30 PM
akhuang created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 4:30 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I'm not familiar with this attribute so leave approval to other more versed with this attribute.

libcxx/include/__config
1346

I would prefer _LIBCPP_STANDALONE_DEBUG to improve readability.

akhuang removed 1 blocking reviewer(s): Restricted Project.Mar 17 2021, 1:30 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 17 2021, 1:30 PM
akhuang removed 1 blocking reviewer(s): Restricted Project.Mar 17 2021, 1:30 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 17 2021, 1:30 PM
akhuang updated this revision to Diff 331358.Mar 17 2021, 1:31 PM

Change macro name to LIBCPP_STANDALONE_DEBUG

akhuang marked an inline comment as done.Mar 30 2021, 11:06 AM

ping?

ldionne requested changes to this revision.Mar 30 2021, 3:33 PM

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

This revision now requires changes to proceed.Mar 30 2021, 3:33 PM

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

Thanks! I tried out the other patch and it does fix the issues with __list_node.

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

Thanks! I tried out the other patch and it does fix the issues with __list_node.

Thanks for trying it out. Depending on the outcome of the discussion currently happening on D99624, I would suggest that we follow a similar path for the other types instead of "hiding" the issue with the attribute.

If it is instead the case that D99624 isn't actually fixing any UB (see discussion over there), then I believe it would be OK to move forward with this patch.

Also, how do you reproduce the issue you're fixing here? Is there a reasonably easy way to do that so we could add something to the test suite?

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

Thanks! I tried out the other patch and it does fix the issues with __list_node.

Thanks for trying it out. Depending on the outcome of the discussion currently happening on D99624, I would suggest that we follow a similar path for the other types instead of "hiding" the issue with the attribute.

If it is instead the case that D99624 isn't actually fixing any UB (see discussion over there), then I believe it would be OK to move forward with this patch.

Also, how do you reproduce the issue you're fixing here? Is there a reasonably easy way to do that so we could add something to the test suite?

Sounds good -

So, to reproduce, I've basically been building a cpp file with stuff like

#include <list>
std::list<int> L {1, 2, 3};
# ..and all the other types

int main() { return 0; }

and using gdb to pretty print the types. (or, build the obj file and look in the dwarf dump to see if the __list_node type is a declaration or not)

It looks like there's already a pretty printer test in libc++ (./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp). It currently fails on my machine for some reason before running the actual tests, but in theory if it worked, it would catch the issues when -fuse-ctor-homing is enabled.

It looks like there's already a pretty printer test in libc++ (./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp). It currently fails on my machine for some reason before running the actual tests, but in theory if it worked, it would catch the issues when -fuse-ctor-homing is enabled.

Actually, it looks like the ./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp test is working; it's just that when it's run with lit, it doesn't show any details about which parts pass or fail.

EricWF accepted this revision.Apr 2 2021, 12:31 PM

I like this patch. It's targeted and fixes the problem it's aiming at well.

Debug information provides real value to our users. And while it might be ideal to fix the actual implementations so they contain less magic and generate better debug info, that's unrealistic given the nature of the standard library (which needs magic from time to time).

This patch LGTM. I don't think it precludes other improvements to the types as @ldionne was mentioning.

akhuang updated this revision to Diff 335686.Apr 6 2021, 5:48 PM

Update a test case to test the use of this attribute.

I've updated the ./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp test so that it runs with constructor homing enabled. One of the cases in the test was already failing, but everything else seems to work. (It only passes if it's run with a version clang that includes my __attribute__((standalone_debug)) change, though).

akhuang updated this revision to Diff 342775.Tue, May 4, 9:31 AM

Update to only apply attribute to types not covered in
https://reviews.llvm.org/D101206.

Also remove test case changes, which don't work because the test could be using
an older version of clang.

akhuang updated this revision to Diff 342908.Tue, May 4, 4:43 PM

put attribute back on the other types

@EricWF @ldionne
I'm thinking it'd be better to commit this patch first, since it seems like fixing the UB otherwise is nontrivial and might take some more time.

If we also want to fix any UB in libc++ we can do that too and then revert this change once it's fixed, but since this solves the issue for the constructor homing debug info optimization it'd be nice to get this in for now.

thoughts?

rnk added a comment.Wed, May 5, 12:37 PM

Yes. I think Amy has demonstrated that we need this attribute, at least for some complex cases. If libc++ is going to use the attribute in at least one place, I don't think there is much cost to adding it in the other places that we think we could address with unions. The attribute can always be removed later if the alleged UB is resolved.

Our motivation is to reduce object file size by 50%, not to address the UB, and this has been the blocking issue for us since October. @EricWF already indicated that he prefers the attribute. @ldionne, you indicated that you'd prefer to avoid the attribute, but if it is unavoidable in some cases, do you have any other objections, or is this OK?

rnk added a comment.Tue, May 11, 10:47 AM

In the absence of more input, I think we should go ahead and land the patch. We have one approval already.

Quuxplusone accepted this revision.Tue, May 11, 11:03 AM
Quuxplusone added a subscriber: Quuxplusone.

FWIW, I think it would be appropriate to wait for @ldionne's OK.
(But I also think it would be appropriate for @ldionne to give his OK. :) This PR seems unobjectionable to me.)

Hm, ok, I'll wait a few days to see if @ldionne has any thoughts on this, and if not, then I think I'll just commit it.

To summarize the reasoning for going with using this attribute rather than fixing libc++: based on comments from the other patch, it seems like changing libc++ could cause more issues. Also, I couldn't find a way to apply the fix to value_type and hash_value_type, and if we have to use the attribute for those types then we might as well use it on all of them. This patch addresses the issues we have with debug info; while fixing any potential undefined behavior would be nice, it looks like it's not as straightforward to fix as we'd like.