This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add _LIBCPP_NODEBUG to global std::ranges::__cpo variables
AbandonedPublic

Authored by Michael137 on Jan 31 2023, 10:11 AM.

Details

Reviewers
aprantl
var-const
ldionne
Group Reviewers
Restricted Project
Summary

The motivation is to avoid cluttering LLDB's global variable view for
std::ranges users.

Before:

(lldb) frame var -g
...
(const std::ranges::__end::__fn) std::__1::ranges::__cpo::end = {}
(const std::ranges::views::__all::__fn) std::__1::ranges::views::__cpo::all = {}
(const std::ranges::__begin::__fn) std::__1::ranges::__cpo::begin = {}
(const std::ranges::views::__take::__fn) std::__1::ranges::views::__cpo::take = {}
(const std::ranges::__max_element::__fn) std::__1::ranges::__cpo::max_element = {}
(const std::ranges::__size::__fn) std::__1::ranges::__cpo::size = {}
(const std::ranges::__data::__fn) std::__1::ranges::__cpo::data = {}

After this patch none of these __cpo variables would show, since no
debug info is available.

Diff Detail

Event Timeline

Michael137 created this revision.Jan 31 2023, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 10:11 AM
Michael137 requested review of this revision.Jan 31 2023, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 10:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Just double-checking: This doesn't affect LLDB's ability to use range expressions in the expression evaluator, right?

Just double-checking: This doesn't affect LLDB's ability to use range expressions in the expression evaluator, right?

Fair point, looks like evaluating ranges expressions currently doesn't quite work. Once that's fixed will check how this patch affects expression evaluation

ldionne accepted this revision.Feb 28 2023, 12:29 PM

This LGTM since it makes the status quo better. I would suggest we make this change right now, and if we need to use a different approach to fix the expression evaluator in the future, then we can cross that bridge when we get to it. But as far as we're concerned right now, this seems to be a pure improvement over the status quo.

Can you please rebase onto main? Once this is merged, we will also want to cherry-pick onto release/16.x.

This revision is now accepted and ready to land.Feb 28 2023, 12:29 PM

This LGTM since it makes the status quo better. I would suggest we make this change right now, and if we need to use a different approach to fix the expression evaluator in the future, then we can cross that bridge when we get to it. But as far as we're concerned right now, this seems to be a pure improvement over the status quo.

Can you please rebase onto main? Once this is merged, we will also want to cherry-pick onto release/16.x.

@ldionne Thanks for the review

Just to clarify, if we want to get expression evaluation to work we will have to remove these attributes again, since calling these function objects requires debug-info for these variables to be present. But that feature will take some time to complete and is unlikely to make it into this release. So if we're fine with the churn for the immediate benefit then I'll go ahead and land this

Just to clarify, if we want to get expression evaluation to work we will have to remove these attributes again, since calling these function objects requires debug-info for these variables to be present. But that feature will take some time to complete and is unlikely to make it into this release. So if we're fine with the churn for the immediate benefit then I'll go ahead and land this

I am fine with that. I imagine we'll need to add something else to those variables to tell LLDB not to show them in the globals view?

Just to clarify, if we want to get expression evaluation to work we will have to remove these attributes again, since calling these function objects requires debug-info for these variables to be present. But that feature will take some time to complete and is unlikely to make it into this release. So if we're fine with the churn for the immediate benefit then I'll go ahead and land this

I am fine with that. I imagine we'll need to add something else to those variables to tell LLDB not to show them in the globals view?

Actually we decided to just purely handle this in the debugger, in order to not break expression evaluation

Workaround is here: https://reviews.llvm.org/D145245

[Github PR transition cleanup]

If we're not doing this in libc++, can we abandon this review?

Michael137 abandoned this revision.Aug 31 2023, 4:50 PM

We worked around this in xcode