Handle non-integer const generic parameters in debuginfo type names.

[michaelwoerister]

This PR fixes an ICE introduced by #85269 which started emitting const generic arguments for debuginfo names but did not cover the case where such an argument could not be evaluated to a flat string of bits.

The fix implemented in this PR is very basic: If try_eval_bits() fails for the constant in question, we fall back to generating a stable hash of the constant and emit that instead. This way we get a (virtually) unique name and side step the problem of generating a string representation of a potentially complex value.

The downside is that the generated name will be rather opaque. E.g. the regression test adds a function const_generic_fn_non_int<()> which is then rendered as const_generic_fn_non_int<{CONST#fe3cfa0214ac55c7}>. I think it's an open question how to deal with this more gracefully.

I'd be interested in ideas on how to do this better.

r? @wesleywiser

cc @dpaoliello (do you see any problems with this approach?)
cc @Mark-Simulacrum & @nagisa (who I've seen comment on debuginfo issues recently -- anyone else?)

Fixes #86893

[Mark-Simulacrum]

Do you think it makes sense to prefix the hash (e.g., even hx.... instead of 0x....)? That way we avoid it looking like an integer argument/const generic, which may be confusing to folks encountering this...

Otherwise I likely don't have much insight into a good way to actually print out more types appropriately -- it seems like we'd either want to find something general (e.g., see if the Ty of the const implements Debug and try to const-eval that -- seems error prone at least until we have const trait impls)... I think it'd be interesting to see what C++ does here, do you know?

[wesleywiser]

This seems ok to land to me as a short term way to unblock work on const generics but we should make sure we have a way to encode const generic values in type names before shipping that feature. I think that's being tracked in #61486.

cc @oli-obk

[eddyb]

Funnily enough this is pretty much what I suggested in #60705 (comment) - I do agree with @Mark-Simulacrum that they should not look like integers.

One of the thoughts I had was {UKNOWN#fe3cfa0214ac55c7} but I'm not sure what the syntax of the debuginfo type names is like and what limitations exist.

[dpaoliello]

Using a name that differentiates it from an integer is a good idea - both to avoid user confusion and prevent collisions.

I'd prefer if we picked a name that somehow indicated that it is a const generic (and not a weirdly named type), maybe const$fe3cfa0214ac55c7, const_generic$fe3cfa0214ac55c7, or complex_const$fe3cfa0214ac55c7?

[eddyb]

The hash is of the const value, so I wouldn't use "const generic", but sadly UKNOWN_CONST$fe3cfa0214ac55c7 is a bit long. Maybe I could be fine without "unknown" in there. The uppercase, btw, is because that's how const items are typically named in Rust, but it's purely an aesthetic choice.

[JulianKnodt]

Not sure if I really have a ton of context here, but would it be helpful to also add the type?
i.e. CONST_<Type>$32BitHash. For more complex const values, I imagine hashing would always be necessary, but having some indicator of the type of the const would be useful?

[michaelwoerister]

Thanks for all the feedback!

I like {CONST#fe3cfa0214ac55c7} (and CONST$fe3cfa0214ac55c7 for cpp_like_names). I was a bit worried about having something there that doesn't parse as an integer but it turns out that C++ allows all kinds of things as non-type template parameters, so we should be fine.

I think it'd be interesting to see what C++ does here, do you know?

C++ doesn't really seem to allow complex constant values as template argument. It does support pointers and references to such objects. Here's an example: https://godbolt.org/z/718M69TGv. Unfortunately, I wasn't able to get godbolt to print debuginfo but when compiling this locally, I get Foo<((const Bar&)(& BAR))> from GCC, Foo<&BAR> from both Clang and MSVC.

So GCC does add the type (as @JulianKnodt suggests) while the others don't. Since the length of these names seems to affect performance during later compilation steps, I'd opt for not adding the type.

[eddyb]

For more complex const values, I imagine hashing would always be necessary, but having some indicator of the type of the const would be useful?

Not really, see #61486 - tho my preferred design doesn't seem to be explained as part of the discussion.

I also wish #83234 had more details, but I can try to explain these "(pure) value trees".

We only want to ever allow type-level constants (such as those passed to const params) to be one of:

1. integer leaves (iN, uN, and also bool and char)

• if we allow raw pointers, they would only be allowed here, with integer addresses
• maybe str and [u8] would also go in here for efficiency, unsure yet about that

2. ADTs of other type-level constants (struct, enum, tuples, arrays, slices, etc.)

• no C unions, since we can't guarantee we can describe them as pure values
• any user-defined ADTs have to have be "structurally matchable" (i.e. auto-derived PartialEq+Eq)
• arguably this includes &T too - even if they are normally thought of as indirection, at the type-level &value acts as a newtype of value, as &T has structural equality that makes it isomorphic to T

(cc @oli-obk - did I miss anything?)

We already have the leaves, what's lacking in the mangling is the second part (the ADTs) - it's not that much design space, since e.g. enum variants can be treated as just structs (i.e. we encode the path to the variant and pretend it's a struct path) - the tricky parts how much aesthetic detail we put in, like whether we encode field names, how differently to encode tuple structs (make them like regular structs or like tuples?), whether we add a special case for str to avoid encoding it as an array, etc. etc.

[oli-obk]

I also wish #83234 had more details, but I can try to explain these "(pure) value trees".

I'm doing that in rust-lang/rustc-dev-guide#1097, but I should probably at least document the ValTree type appropriately

[michaelwoerister]

I've update the PR to emit integers and booleans directly and everything else gets emitted as {CONST#fe3cfa0214ac55c7} (or CONST$fe3cfa0214ac55c7 for MSVC). Note that the GDB version on CI does not actually print the names we generate here. Instead it demangles the symbol name which, because it's still using the legacy scheme, does not contain any generic arguments at all. Newer GDB versions do seem to use the names from the DW_AT_name field.

[oli-obk]

r=me unless you want to wait on other reviews

[wesleywiser]

@bors r=oli-obk,wesleywiser rollup

[bors]

📌 Commit da56618 has been approved by oli-obk,wesleywiser

[michaelwoerister]

Fixed the test case.

@bors r=oli-obk,wesleywiser rollup=never

[bors]

📌 Commit a4e24c6149c47922dbb5e7cf345cd35c05881edf has been approved by oli-obk,wesleywiser

[bors]

⌛ Testing commit a4e24c6149c47922dbb5e7cf345cd35c05881edf with merge d2a86a10d3d68ade9ac91fae90b0a5dd57d13b9f...

[bors]

💔 Test failed - checks-actions

[michaelwoerister]

Looks like CDB prints the functions in a different order now.

[michaelwoerister]

@bors r=oli-obk,wesleywiser rollup=never

[bors]

📌 Commit 28343be has been approved by oli-obk,wesleywiser

[bors]

⌛ Testing commit 28343be with merge 4f0c568...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk,wesleywiser
Pushing 4f0c568 to master...