Add `debug_printf!` and `debug_printfln!` macros that uses the DebugPrintf extension #768

expenses · 2021-10-16T17:58:22Z

This will probably require some work before being merged. These macros and different from the core rust print and println macros as the type of variables needs to be specified in the format string. There's probably a way to change this as mentioned here: #151 (comment) but for now I'm doing the simpler thing.

I have a branch here: main...expenses:printf-working that modifies the sky shader and ash runner for testing. It requires 'Debug Printf' to be checked in the Vulkan Configuration app.

One important caveat is that using DebugPrintf seems to SEGFAULT (on Windows I get a (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)) the Vulkan program if you build a single module with multiple entry points. This is truely bizarre, no clue what's going on here. The only difference between working code and segfaulting code is this: expenses/rust-gpu@expenses:printf-not-working...printf-working.

At first I thought this was a driver issue but I'm getting this on both an integrated intel gpu and an external amd gpu.

This reverts commit 6a21a29.

… blocking the in(reg)

expenses · 2021-10-16T18:24:11Z

Actually, I do get this validation error so it's not like the segfault is coming out of nowhere:
[ERROR] [Debug Msg][validation] "Validation Error: [ UNASSIGNED-Debug-Printf ] Object 0: handle = 0x24b805bb370, name = Device, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0x9472fbd3 | Error during shader instrumentation: line 0: Mixed stage shader module not supported"

expenses · 2021-10-16T18:32:21Z

Fun fact: you can print emojis using these macros. I was pleasantly surprised that unicode didn't get mangled somehow.

khyperia

It would be nice to add some sort of check on vulkan to avoid the "Mixed stage shader module not supported" obscure issue, seems pretty confusing especially if validation layers are disabled.

It's also rather unfortunate that we can't do typechecking on the format string, that really sucks, but oh well I guess.

Also, I have a vague preference to call it debug_printf, but I could go either way.

Out of curiosity, what's the error message when calling it outside of an unsafe block?

crates/spirv-std/macros/src/lib.rs

tests/ui/arch/printf.rs

expenses · 2021-10-18T10:54:59Z

It's also rather unfortunate that we can't do typechecking on the format string, that really sucks, but oh well I guess.

I think we probably could do some kind of typechecking. If we had a function like this:

fn assert_is_type<T>(ty: T) -> T {
  ty
}

we could map each format argument to the rust type - %f to f32 for example - and wrap each one in assert_is_type::<T> before using them. It's a bit gross but it would work.

What I believe we can't do is any kind of type inference.

Also, I have a vague preference to call it debug_printf, but I could go either way.

👍

Out of curiosity, what's the error message when calling it outside of an unsafe block?

It's the same as calling an asm! macro outside of unsafe - use of inline assembly is unsafe and requires unsafe function or block

expenses · 2021-10-18T16:35:47Z

we could map each format argument to the rust type - %f to f32 for example - and wrap each one in assert_is_type::<T> before using them. It's a bit gross but it would work.

I did a test of this (just assuming every format argument is f32) and it actually works quite nicely:

error[E0308]: mismatched types
  --> $DIR/debug_printf.rs:41:31
   |
41 |         debug_printfln!("%f", vec);
   |                               ^^^ expected `f32`, found struct `Vec2`

Commit: ~~expenses@a76222b~~ updated: expenses@b545f6b

…h functions

…ewlines

khyperia

fantastic! feel free to merge the typechecking, too, that'd be really nice to have.

expenses · 2021-10-21T08:15:37Z

crates/spirv-std/macros/src/lib.rs

+            quote::quote! { spirv_std::debug_printf_assert_is_vector::<#ty, _, #count> }
+        } else {
+            let ty = map_specifier_to_type(specifier);
+            quote::quote! { spirv_std::debug_printf_assert_is_type::<#ty> }


I don't like that this assumes that spirv_std is in scope like this. Not sure what else we can do though.

Seems like it's an open issue rust-lang/rust#54363

khyperia · 2021-10-21T11:27:23Z

crates/spirv-std/macros/src/lib.rs

+            "ul" => quote::quote! { u64 },
+            "lu" => quote::quote! { u64 },
+            "lx" => quote::quote! { u64 },
+            _ => unreachable!(),


~~I just realized, there's no support here for f64 or int sizes under 32 bits - I would have expected there to be support, where are you finding the docs for what specifiers correspond to what types?~~ sorry, missed the bit in the linked document

Oh haha, I actually missed the

No length modifiers. Everything except ul, lu, and lx is 32 bits, and ul and lx values are printed in hex

line when I read the document before, it's good to know that this is doing things correctly.

expenses · 2021-10-21T12:20:06Z

I had to change how this does argument count checking, as

debug_printf!("%r", 11_i32);

was being accepted as invalid before when it shouldn't have been. I also added a compile-fail test to check this.

We have another slight problem though that it won't accept

debug_printfln!("%%r %%f");

as valid even though it should. I'm not sure if we can change the regex to check for an odd number of %s (as %% is treated as a single % so %%%f is treated as %<f>). The solution to this is to probably capture all the %s and filter out matches with an even number of them.

This is getting into edge-cases though. Not sure how much effort should be put into those.

khyperia · 2021-10-21T12:26:06Z

Is it actually specified/defined what the behavior of debug_printf!("%r") is? (as opposed to "%%r")

As for parsing validity, the proper solution here is probably to drop the regex and do lexing ourselves - regex probably isn't the right tool for this job.

This is getting into edge-cases though. Not sure how much effort should be put into those.

Compilers are entirely about edge cases, this is really important to get right~

expenses · 2021-10-21T12:47:43Z

Is it actually specified/defined what the behavior of debug_printf!("%r") is? (as opposed to "%%r")

Just tested this - it just prints r without a %.

As for parsing validity, the proper solution here is probably to drop the regex and do lexing ourselves - regex probably isn't the right tool for this job.

Yeah, it almost certainly isn't.

Compilers are entirely about edge cases, this is really important to get right~

For sure :) but right now, I don't think anyone would really mind if they had to do "%% %f..." (with a gap) instead of "%%%f...". But as long as you're okay with these macros having a fair but of validation code, then I'm happy to work on that.

khyperia · 2021-10-21T13:18:00Z

Just tested this - it just prints r without a %.

what in the absolute heckity goodness, lmao, that's wild

anyway, yeah, probably fine to merge as-is and work on full validation later if you'd prefer that PR model!

as for the path test failure, I thiiink there might be a way to work around that with something like this, although that might have to be tweaked slightly (not sure - might be fine copypasting that as-is? - it's been a long time since I've dealt with a failure like that)

expenses · 2021-10-21T14:17:38Z

what in the absolute heckity goodness, lmao, that's wild

C does the same thing actually 😅😅😅 at least you get a warning

x.c:4:11: warning: invalid conversion specifier 'r' [-Wformat-invalid-specifier]
        printf("%r\n");
                ~^
1 warning generated.

I do generally prefer making more small PRs but here I'd like to work on the validation more before merging. Is there any way of lexing that would be best or you'd prefer? I feel like just iterating over the string and doing a bit of look-ahead would work fine.

khyperia · 2021-10-21T14:28:36Z

Is there any way of lexing that would be best or you'd prefer? I feel like just iterating over the string and doing a bit of look-ahead would work fine.

Yeah, sounds fine! There's the bit of lexing in this project I did here, something like that, or whatever lookahead stuff you think might make things cleaner.

…t types

expenses · 2021-10-21T15:48:15Z

Okay, I've got something that works quite well now. It could be a bit more concise though.

khyperia · 2021-10-22T09:01:11Z

heck yeah, that looks super good! thanks so much for doing that~

khyperia · 2021-10-23T08:05:13Z

🎉

expenses added 8 commits October 16, 2021 06:47

Add a very bad no good prinfln macro

24fafec

Use a better registry identifier

5656770

Do a little bit of argument count checking

9931531

Use OpLoads to be able to print vectors

4f2048d

Add an error for string literal expressions

6a21a29

Revert "Add an error for string literal expressions"

2ee529e

This reverts commit 6a21a29.

Add both printf and printfln, allow for other kinds of expressions by…

9311710

… blocking the in(reg)

Tidy up a little

a6c29fe

expenses changed the title ~~Add printf and printfln macros that uses the DebugPrintf extension~~ Add printf! and printfln! macros that uses the DebugPrintf extension Oct 16, 2021

expenses added 2 commits October 17, 2021 10:49

Add a compiletest

29481ba

Run rustfmt

1f68124

expenses mentioned this pull request Oct 17, 2021

SPV_KHR_non_semantic_info: support for mixed stage shader modules? KhronosGroup/SPIRV-Registry#131

Closed

expenses marked this pull request as ready for review October 17, 2021 20:29

expenses requested review from eddyb and khyperia as code owners October 17, 2021 20:29

khyperia reviewed Oct 18, 2021

View reviewed changes

crates/spirv-std/macros/src/lib.rs Outdated Show resolved Hide resolved

crates/spirv-std/macros/src/lib.rs Outdated Show resolved Hide resolved

tests/ui/arch/printf.rs Outdated Show resolved Hide resolved

Rename to debug_printf

d095849

expenses added 5 commits October 18, 2021 04:04

Parse variables using punctated

2f4f9c9

Handle inner quotes

aecdf5b

Add tests for function and method calls

564165f

Escape only quote marks

fb6f66c

Run rustfmt

0f0d13d

expenses requested a review from khyperia October 18, 2021 11:39

expenses added 3 commits October 19, 2021 01:10

Use a regex to parse types from the format string and assert them wit…

b545f6b

…h functions

Add extra tests, escape the string correctly while keeping printfln n…

42978ab

…ewlines

Use the string debug impl after all

a0f4615

khyperia approved these changes Oct 21, 2021

View reviewed changes

expenses added 2 commits October 21, 2021 01:09

Cargo fmt

c59e3ce

Merge branch 'printf-macro-type-checking' into printfln-macro

53ae4a1

expenses commented Oct 21, 2021

View reviewed changes

expenses requested a review from khyperia October 21, 2021 08:16

expenses added 2 commits October 21, 2021 01:49

Cargo fmt again

ffc7505

fmt compiletest

81d4103

khyperia reviewed Oct 21, 2021

View reviewed changes

khyperia approved these changes Oct 21, 2021

View reviewed changes

expenses added 2 commits October 21, 2021 04:48

Merge match branches

81ad59e

Improved argument checking

602b691

Filter out captures like %%f

8c76f59

Use a peekable character iterator to parse the format string into rus…

6fbde8e

…t types

continue on %% to unindent the code

2836b3e

expenses added 3 commits October 22, 2021 03:15

Finishing touches

7155e3b

Rustfmt

aa6345f

Re-bless compile tests

41ea76e

expenses changed the title ~~Add printf! and printfln! macros that uses the DebugPrintf extension~~ Add debug_printf! and debug_printfln! macros that uses the DebugPrintf extension Oct 22, 2021

Only last tiny change: Replace impl Iterator with std::str::Chars

10575d3

khyperia merged commit e5c2953 into EmbarkStudios:main Oct 23, 2021

charles-r-earp mentioned this pull request Apr 24, 2023

DebugPrintf prevents panics' infinite loops from being (unsoundly) optimized away. #1048

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `debug_printf!` and `debug_printfln!` macros that uses the DebugPrintf extension #768

Add `debug_printf!` and `debug_printfln!` macros that uses the DebugPrintf extension #768

expenses commented Oct 16, 2021

expenses commented Oct 16, 2021

expenses commented Oct 16, 2021 •

edited

Loading

khyperia left a comment

expenses commented Oct 18, 2021

expenses commented Oct 18, 2021 •

edited

Loading

khyperia left a comment

expenses Oct 21, 2021

khyperia Oct 21, 2021

khyperia Oct 21, 2021 •

edited

Loading

expenses Oct 21, 2021

expenses commented Oct 21, 2021

khyperia commented Oct 21, 2021 •

edited

Loading

expenses commented Oct 21, 2021 •

edited

Loading

khyperia commented Oct 21, 2021 •

edited

Loading

expenses commented Oct 21, 2021

khyperia commented Oct 21, 2021

expenses commented Oct 21, 2021 •

edited

Loading

khyperia commented Oct 22, 2021

khyperia commented Oct 23, 2021

Add debug_printf! and debug_printfln! macros that uses the DebugPrintf extension #768

Add debug_printf! and debug_printfln! macros that uses the DebugPrintf extension #768

Conversation

expenses commented Oct 16, 2021

expenses commented Oct 16, 2021

expenses commented Oct 16, 2021 • edited Loading

khyperia left a comment

Choose a reason for hiding this comment

expenses commented Oct 18, 2021

expenses commented Oct 18, 2021 • edited Loading

khyperia left a comment

Choose a reason for hiding this comment

expenses Oct 21, 2021

Choose a reason for hiding this comment

khyperia Oct 21, 2021

Choose a reason for hiding this comment

khyperia Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

expenses Oct 21, 2021

Choose a reason for hiding this comment

expenses commented Oct 21, 2021

khyperia commented Oct 21, 2021 • edited Loading

expenses commented Oct 21, 2021 • edited Loading

khyperia commented Oct 21, 2021 • edited Loading

expenses commented Oct 21, 2021

khyperia commented Oct 21, 2021

expenses commented Oct 21, 2021 • edited Loading

khyperia commented Oct 22, 2021

khyperia commented Oct 23, 2021

Add `debug_printf!` and `debug_printfln!` macros that uses the DebugPrintf extension #768

Add `debug_printf!` and `debug_printfln!` macros that uses the DebugPrintf extension #768

expenses commented Oct 16, 2021 •

edited

Loading

expenses commented Oct 18, 2021 •

edited

Loading

khyperia Oct 21, 2021 •

edited

Loading

khyperia commented Oct 21, 2021 •

edited

Loading

expenses commented Oct 21, 2021 •

edited

Loading

khyperia commented Oct 21, 2021 •

edited

Loading

expenses commented Oct 21, 2021 •

edited

Loading