[libcxx] Add _LIBCPP_NODEBUG to std::conditional related typedfs

[dblaikie]

Summary

Looks like std::conditional wasn't included in 14d4869209cc8ff9a53aaa5a59d6409fbc1c5f5d
(& maybe other typedefs that should be using this technique either got
missed or have regressed since that change was made)

This was noticed by a 1.4% clang.dwp regression due to
f4fb72e6d4cee1097e6606e66232fe55e793cd86 introducing more instantiations
of std::conditional - this change reduces that regression to 0.6% at
least.

I'm also looking at other instantiations caused by that change that
might be able to be addressed - but a quick grep shows ~200 "type"
typedefs missing _LIBCPP_NODEBUG, so maybe a systematic application of
the typedef might be suitable?

[dblaikie]

rebase

[Mordante]

Thanks for the patch, LGTM!

! @dblaikie wrote:
I'm also looking at other instantiations caused by that change that
might be able to be addressed - but a quick grep shows ~200 "type"
typedefs missing _LIBCPP_NODEBUG, so maybe a systematic application of
the typedef might be suitable?

I had a quick look and we indeed don't use this macro often and could use it more.
I've mentioned this patch on the #libcxx Discord channel so we can discuss it there further.

[philnik]

I don't think we should strip debug information in the public API. Instead, we should try to remove as much implementation-detail debug-info as possible.

[dblaikie]

In D131082#3721806, @philnik wrote:

I don't think we should strip debug information in the public API. Instead, we should try to remove as much implementation-detail debug-info as possible.

Hmm, I can't find the internal or external discussions related to the previous work in this direction, but I was part of them & it was a direction I initially proposed that was implemented here: https://github.com/llvm/llvm-project/commit/14d4869209cc8ff9a53aaa5a59d6409fbc1c5f5d

I'm still generally in favor of that direction (as someone who's spent years working on debug info size issues) - the type traits don't generally add value to a debugger/user of a debugger - being able to look straight through the trait and see the underlying type rather than being presented with the trait as the type is, I believe, generally a better user experience and a better size experience.

Do you have use cases where you find the trait info helpful, as a debug-info user?

[ldionne]

Could someone share something like a screenshot of the debugging experience before and after? I'm also kind of uneasy about removing debug information, but I think it might be because I don't properly understand what effect it has on the debugging experience. @dblaikie you have a lot more experience with this than I do, so I assume I'd share your opinion if I saw what it looked like.

Requesting changes so I'm notified of any update.

libcxx/include/__type_traits/conditional.h

39–45

[dblaikie]

In D131082#3792754, @ldionne wrote:

Could someone share something like a screenshot of the debugging experience before and after? I'm also kind of uneasy about removing debug information, but I think it might be because I don't properly understand what effect it has on the debugging experience. @dblaikie you have a lot more experience with this than I do, so I assume I'd share your opinion if I saw what it looked like.

Requesting changes so I'm notified of any update.

Yeah, for sure - I've been meaning to gather more of the size data too, but that's a bit more involved.

A rough explanation, based on what I can test:

1. for alias templates both LLVM and GCC's debug info is a bit broken-ish. GCC produces "simplified" template names (not including any template parameters in the names) and Clang's currently producing duplicate alias template definitions depending on the spelling of the parameter (eg: conditional_t<false, A, B> is a distinct bit of debug info from conditional_t<0, A, B> - fixing this I guess will significantly reduce debug info size, they should probably both canonicalize to conditional_t<false, A, B>).

2. gdb at least looks through/appears to ignore the typedefs entirely (if you ptype x where x is of some conditional_t type, it shows the underlying type (eg: A or B in the examples above), no mention of conditional)

3. Even if a debugger does render the conditional_t specialization (lldb does in some cases - "member reference base type 'conditional_t<true, double, float>' (aka 'double') is not a structure or union") once (1) is fixed, you only get the canonical representation of the type, so you don't get a lot of information - like the expression that was used for condition.

Debuggers could benefit from other typedefs - like knowing that some int* is actually a vector<int>::iterator might be helpful for a user - but type traits I don't think provide the same sort of value (eg: "remove_const<const T> (aka ("T"))" doesn't add a lot of value for the developer, etc) - which was the conclusion we reached/the basis on which the plumbing (& the uses that are already in-tree) for this was added previously.

libcxx/include/__type_traits/conditional.h

39–45

Should be do the typedef->using as a separate change? Happy to send another patch.

[dblaikie]

(I can provide more detailed example/quotes from debuggers too - but after looking I didn't find really good examples so far - I can look further)

[EricWF]

In D131082#3792754, @ldionne wrote:

Could someone share something like a screenshot of the debugging experience before and after? I'm also kind of uneasy about removing debug information, but I think it might be because I don't properly understand what effect it has on the debugging experience. @dblaikie you have a lot more experience with this than I do, so I assume I'd share your opinion if I saw what it looked like.

Requesting changes so I'm notified of any update.

Let me try and convince you by way of analogy. The question, as I see it, is whether for a computed value v, we want the debugger to show us the expression that computed that value, or the value itself. Let's consider this question for a normal, non-metaprogramming, conditional expresssion. For example

void foo(int x) {
  // BREAKPOINT
}
int main(int argc, char** argv) {
  foo(argc == 0 ? 42 : 101);
}

Now imagine I'm debugging the call to foo and I want to know the value of x, so I ask the debugger to print the variable and I get

(lldb) var
> (int) x = 101

But imagine if instead the debugger printed the expressions used to compute x. I.e.

(lldb) var
`(int) x = false ? 42 : 101`

I already know the expression used to produce y from reading the source, and asking me to compute the value myself from intermediate expressions is not helpful. And as the source expressions get more complex, the output from the debugger becomes less and less useful.

Unfortunately the latter option is what we're providing to users since we don't apply _LIBCPP_NODEBUG to conditional.

Here's a concrete (but silly) example:

debug.cpp

// forward the value as is, unless it's a c-string, then forward as a string_view.
template <class T>
auto my_forward(typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type v) {
// BREAKPOINT
// (lldb) var
 return v;
}

int main() {
  my_forward<int>(42);
}

Currently, when I step to the breakpoint and print variables at the breakpoint, I get

* thread #1, name = 'debug.out', stop reason = step in
    frame #0: 0x0000555555555197 debug.out`auto my_forward<int>(v=42) at debug.cpp:10:10
   7   	// forward the value as is, unless it's a c-string, then forward as a string_view.
   8   	template <class T>
   9   	auto my_forward(typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type v) {
-> 10  	  return v;
    	         ^
   11  	}
   12  	
   13  	int main(int argc, char** argv) {
...
(lldb) var
(std::conditional<false, std::basic_string_view<char, std::char_traits<char> >, int>::type) v = 42

However, with this change, the debugger prints the following

* thread #1, name = 'debug.out', stop reason = step in
    frame #0: 0x0000555555555197 debug.out`auto my_forward<int>(v=42) at debug.cpp:10:10
   7   	// forward the value as is, unless it's a c-string, then forward as a string_view.
   8   	template <class T>
   9   	auto my_forward(typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type v) {
-> 10  	  return v;
    	         ^
   11  	}

(lldb) var
(int) v = 42

Notice that in both cases, the debugger prints the source code, providing enough context for the reader.

I believe this change not only significatly improves debug binary bloating, but also improves the debugging experience for our users.
And that we should apply _LIBCPP_NODEBUG aggressively to libc++'s typedefs, including inside conditional.

@ldionne Does this help answer your questions.

This change LGTM, but I'll let @ldionne stamp it for libc++.

[ldionne]

Thanks a lot both for the explanation and sorry this fell under the radar. On the surface, I think this looks like a pure improvement on all aspects, in that case.

Just before we go forward, is there any way to still see the full type of the variable (i.e. std::conditional<false, std::basic_string_view<char, std::char_traits<char> >, int>::type), or is that information lost forever? If it's lost forever, basically one would have to substitute the various types by hand into the std::conditional template?

// We know this from the fact that we're in my_forward<int>:
T = int

// So we manually substitute:
   typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type
-> typename std::conditional<std::is_same<int, const char*>::value, std::string_view, int>::type
-> typename std::conditional<false, std::string_view, int>::type // this is roughly what we would get without _LIBCPP_NODEBUG_TYPE

Is there an easier way to figure out that type in the debugger, or not? I don't think it's a dealbreaker if not, but I'd like to understand.

libcxx/include/__type_traits/conditional.h

39–45

I really don't mind.

[ldionne]

Re std::conditional<false, std::basic_string_view<char, std::char_traits<char> >, int>::type, a private discussion w/ @EricWF made me realize there was not much use to this. I'm fine with this.

[ldionne]

Just to clarify: I'm fine with this because it provides an improvement in the debugging experience and an improvement of debug info sizes. If we were worsening the debug experience to improve debug info size, it would be a different discussion.

[ldionne]

FWIW @EricWF and I met offline just now and we agreed that it would probably improve the debugging experience to apply _LIBCPP_NODEBUG_TYPE more liberally to typedefs that represent computational things. We'd want to be careful about things that are alternate names for types, for example std::size_t and Container::value_type. So we'll have to consider each application individually for its own merits, but we both agree that it probably makes sense to apply it more liberally.

[ldionne]

Gentle ping -- let's apply any remaining comments, rebase this to get CI to run and then land this!

[dblaikie]

Use using decls while we're here

[dblaikie]

In D131082#3944183, @ldionne wrote:

Gentle ping -- let's apply any remaining comments, rebase this to get CI to run and then land this!

Ah, thanks! I lost track of this somewhere along the way. Updated & waiting for the CI to cycle.

[ldionne]

In D131082#3944659, @dblaikie wrote:

In D131082#3944183, @ldionne wrote:

Gentle ping -- let's apply any remaining comments, rebase this to get CI to run and then land this!

Ah, thanks! I lost track of this somewhere along the way. Updated & waiting for the CI to cycle.

It happens to the best of us :-)

Thanks for the patch!

[ldionne]

The CI failures are flakes. We've been having a bit of trouble recently. This is good to land.

[dblaikie]

In D131082#3945056, @ldionne wrote:

The CI failures are flakes. We've been having a bit of trouble recently. This is good to land.

ah, thanks for the info!

libcxx/include/__type_traits/conditional.h

struct _IfImpl<false> {
   template <class _IfRes, class _ElseRes>
   using _Select _LIBCPP_NODEBUG = _ElseRes;
 };
 
 template <bool _Cond, class _IfRes, class _ElseRes>
 using _If _LIBCPP_NODEBUG = typename _IfImpl<_Cond>::template _Select<_IfRes, _ElseRes>;
 
 template <bool _Bp, class _If, class _Then>
-    struct _LIBCPP_TEMPLATE_VIS conditional {typedef _If type;};
+struct _LIBCPP_TEMPLATE_VIS conditional {
+  _LIBCPP_NODEBUG typedef _If type;
+};
 template <class _If, class _Then>
-    struct _LIBCPP_TEMPLATE_VIS conditional<false, _If, _Then> {typedef _Then type;};
+struct _LIBCPP_TEMPLATE_VIS conditional<false, _If, _Then> {
+  _LIBCPP_NODEBUG typedef _Then type;
+};
 
 #if _LIBCPP_STD_VER > 11
 template <bool _Bp, class _IfRes, class _ElseRes>
-using conditional_t = typename conditional<_Bp, _IfRes, _ElseRes>::type;
+using conditional_t _LIBCPP_NODEBUG = typename conditional<_Bp, _IfRes, _ElseRes>::type;
 #endif
 
 // Helper so we can use "conditional_t" in all language versions.
-template <bool _Bp, class _If, class _Then> using __conditional_t = typename conditional<_Bp, _If, _Then>::type;
+template <bool _Bp, class _If, class _Then>
+using __conditional_t _LIBCPP_NODEBUG = typename conditional<_Bp, _If, _Then>::type;
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___TYPE_TRAITS_CONDITIONAL_H