[SYCL][RTC] Add support for errors and build log in JIT-based RTC

[sommerlukas]

Adds support for correct error message routing and the save_log property to the SYCL-RTC implementation based on the SYCL-JIT library.

The diagnostic messages associated with any errors encountered during runtime compilation is stored in the what of the exception thrown.

If the user passes the save_log property defined in the kernel_compiler extension, all diagnostics encountered during compilation (errors, warnings, notes and remarks) are stored in the user-provided string.

[sommerlukas]

This PR currently depends on #16109 for the definition of RTCResult and is therefore marked draft.

[sommerlukas]

@cperkinsintel @jopperm @gmlueck I'd like to hear your opinion on one aspect of the implementation.

In the current implementation, I decided to only include the essence of the error diagnostic message in the what message of the exception.

For the error in the e2e tests, this means that the what message is:

Device compilation failed
Detailed information:
error: use of undeclared identifier 'no'
error: expected ';' at end of declaration
For more information, use the 'save_log' property to obtain the full build log

The build log (via save_log property) contains the full details on the error (and any other diagnostics):

/home/lukas/Code/RTC/upstream/dpcpp/build/rtc.cpp:9:44: error: use of undeclared identifier 'no'
    9 |   sycl::id<1> GId = Item.get_global_id() + no semi colon !!
      |                                            ^
/home/lukas/Code/RTC/upstream/dpcpp/build/rtc.cpp:9:46: error: expected ';' at end of declaration
    9 |   sycl::id<1> GId = Item.get_global_id() + no semi colon !!
      |                                              ^
      |                                              ;

My thinking was that we don't want to include a verbose error message, including other non-error diagnostics, in what of the exception.

If you prefer to have the full build log in what of the exception, I can also implement that.

What's your preference?

[jopperm]

My thinking was that we don't want to include a verbose error message, including other non-error diagnostics, in what of the exception.

That makes sense to me, but would it be possible to include the source location in the short error messages that go into the exception, to make them more useful on their own? We probably need to include the file path as well, unless it is the RTC source file or one of the virtual headers.

[sommerlukas]

but would it be possible to include the source location in the short error messages that go into the exception, to make them more useful on their own

The TextDiagnosticBuffer has SourceLocation, but to turn them into a FullSourceLoc, we would need a SourceManager. To me, it seemed to be a bit of an overkill to set all that up.

[gmlueck]

In the current implementation, I decided to only include the essence of the error diagnostic message in the what message of the exception.

My thinking was that we don't want to include a verbose error message, including other non-error diagnostics, in what of the exception.

The specification addresses this:

An exception with the errc::build error code if the compilation or linking operations fail. In this case, the exception what string provides a full build log, including descriptions of any errors, warning messages, and other diagnostics. This string is intended for human consumption, and the format may not be stable across implementations of this extension.

I found that this was pretty convenient when developing test applications. I didn't need to set up an exception handler or use the build_log property. The default behavior in this case is to terminate the application and print a nice error message that tells me exactly what problem I had in my source string.

Of course, we can change the specification if we think the other behavior is better, but I wonder how easy it is to determine the "essence" of the error. Quite often, a mistake in C++ syntax will lead to a raft of error messages, and I think it is not an easy task to figure out which of these is the essence of the problem.

I also suspect that changing the specification in this way would make the extension more difficult for other vendors to implement. The way it is written now, a vendor can spawn the compiler in a separate process and capture the stdout / stderr into the what string. If a vendor wants to implement the extension by spawning the compiler in a separate process, would it have to parse through the stderr output to find the essence of the error?

[sommerlukas]

In the current implementation, I decided to only include the essence of the error diagnostic message in the what message of the exception.
My thinking was that we don't want to include a verbose error message, including other non-error diagnostics, in what of the exception.

The specification addresses this:

An exception with the errc::build error code if the compilation or linking operations fail. In this case, the exception what string provides a full build log, including descriptions of any errors, warning messages, and other diagnostics. This string is intended for human consumption, and the format may not be stable across implementations of this extension.

I found that this was pretty convenient when developing test applications. I didn't need to set up an exception handler or use the build_log property. The default behavior in this case is to terminate the application and print a nice error message that tells me exactly what problem I had in my source string.

That's a good point, it's probably more convenient to get the full error message right away than having to go back, add the save_log property and recompile and rerun.

Of course, we can change the specification if we think the other behavior is better, but I wonder how easy it is to determine the "essence" of the error. Quite often, a mistake in C++ syntax will lead to a raft of error messages, and I think it is not an easy task to figure out which of these is the essence of the problem.

I also suspect that changing the specification in this way would make the extension more difficult for other vendors to implement. The way it is written now, a vendor can spawn the compiler in a separate process and capture the stdout / stderr into the what string. If a vendor wants to implement the extension by spawning the compiler in a separate process, would it have to parse through the stderr output to find the essence of the error?

So far, I'm simply using the severity of the diagnostic given by Clang/LLVM to decide what should be in what. But you're right, if the compiler does not separate errors and warnings into different streams (e.g., stderr and stdout) or has flags to achieve such behavior, it would be hard for other implementations if we changed the specification.

Based on your points and @jopperm's feedback that source locations would be nice, I'll change the implementation to simply include the entire build log in what (also simplifies our implementation).

I'll update the PR accordingly, thanks for the feedback!

[jopperm]

I addressed the CI errors in #16109, so these should be gone after rebasing.

[victor-eds]

Suggested change

	explicit RTCResult(RTCBundleInfo &&BundleInfo, const char *BuildLog)
	: Failed{false}, BundleInfo{std::move(BundleInfo)}, ErrorMessage{
	BuildLog} {}
	RTCResult(RTCBundleInfo &&BundleInfo, const char *BuildLog)
	: Failed{false}, BundleInfo{std::move(BundleInfo)}, ErrorMessage{
	BuildLog} {}

Drop explicit

[victor-eds]

LGTM but NIT.

[sommerlukas]

Post-commit failure is addressed in #16243.