Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Improve GD.PushError and GD.PushWarning #79280

Merged
merged 1 commit into from
Aug 3, 2023
Merged

C#: Improve GD.PushError and GD.PushWarning #79280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions editor/debugger/script_editor_debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,8 +526,11 @@ void ScriptEditorDebugger::_parse_message(const String &p_msg, const Array &p_da
error->set_custom_color(1, color);

String error_title;
if (oe.callstack.size() > 0) {
// If available, use the script's stack in the error title.
if (!oe.source_func.is_empty() && source_is_project_file) {
// If source function is inside the project file.
error_title += oe.source_func + ": ";
} else if (oe.callstack.size() > 0) {
// Otherwise, if available, use the script's stack in the error title.
error_title = _format_frame_text(&oe.callstack[0]) + ": ";
} else if (!oe.source_func.is_empty()) {
// Otherwise try to use the C++ source function.
Expand Down Expand Up @@ -602,7 +605,10 @@ void ScriptEditorDebugger::_parse_message(const String &p_msg, const Array &p_da
if (i == 0) {
stack_trace->set_text(0, "<" + TTR("Stack Trace") + ">");
stack_trace->set_text_alignment(0, HORIZONTAL_ALIGNMENT_LEFT);
error->set_metadata(0, meta);
if (!source_is_project_file) {
// Only override metadata if the source is not inside the project.
error->set_metadata(0, meta);
}
tooltip += TTR("Stack Trace:") + "\n";
}

Expand Down
71 changes: 65 additions & 6 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/DebuggingUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,46 @@ internal static class DebuggingUtils
{
private static void AppendTypeName(this StringBuilder sb, Type type)
{
if (type.IsPrimitive)
sb.Append(type.Name);
else if (type == typeof(void))
// Use the C# type keyword for built-in types.
// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/built-in-types
if (type == typeof(void))
sb.Append("void");
else if (type == typeof(bool))
sb.Append("bool");
else if (type == typeof(byte))
sb.Append("byte");
else if (type == typeof(sbyte))
sb.Append("sbyte");
else if (type == typeof(char))
sb.Append("char");
else if (type == typeof(decimal))
sb.Append("decimal");
else if (type == typeof(double))
sb.Append("double");
else if (type == typeof(float))
sb.Append("float");
else if (type == typeof(int))
sb.Append("int");
else if (type == typeof(uint))
sb.Append("uint");
else if (type == typeof(nint))
sb.Append("nint");
else if (type == typeof(nuint))
sb.Append("nuint");
else if (type == typeof(long))
sb.Append("long");
else if (type == typeof(ulong))
sb.Append("ulong");
else if (type == typeof(short))
sb.Append("short");
else if (type == typeof(ushort))
sb.Append("ushort");
else if (type == typeof(object))
sb.Append("object");
else if (type == typeof(string))
sb.Append("string");
else
sb.Append(type);

sb.Append(' ');
}

internal static void InstallTraceListener()
Expand Down Expand Up @@ -70,13 +102,26 @@ public unsafe void Dispose()
}
}

internal static unsafe StackFrame? GetCurrentStackFrame(int skipFrames = 0)
{
// We skip 2 frames:
// The first skipped frame is the current method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this always reliably work or can inlining mean that these frames can potentially not exist? (Idk if such frames are available in C#)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a really good point, I don't know.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still a concern or is this not a problem since it's only going to execute in Debug builds which hopefully disable inlining optimizations?

// The second skipped frame is a method in NativeInterop.NativeFuncs.
var stackTrace = new StackTrace(skipFrames: 2 + skipFrames, fNeedFileInfo: true);
return stackTrace.GetFrame(0);
}

[UnmanagedCallersOnly]
internal static unsafe void GetCurrentStackInfo(void* destVector)
{
try
{
var vector = (godot_stack_info_vector*)destVector;
var stackTrace = new StackTrace(skipFrames: 1, fNeedFileInfo: true);

// We skip 2 frames:
// The first skipped frame is the current method.
// The second skipped frame is a method in NativeInterop.NativeFuncs.
var stackTrace = new StackTrace(skipFrames: 2, fNeedFileInfo: true);
int frameCount = stackTrace.FrameCount;

if (frameCount == 0)
Expand All @@ -87,6 +132,14 @@ internal static unsafe void GetCurrentStackInfo(void* destVector)
int i = 0;
foreach (StackFrame frame in stackTrace.GetFrames())
{
var method = frame.GetMethod();

if (method is MethodInfo methodInfo && methodInfo.IsDefined(typeof(StackTraceHiddenAttribute)))
{
// Skip methods marked hidden from the stack trace.
continue;
}

string? fileName = frame.GetFileName();
int fileLineNumber = frame.GetFileLineNumber();

Expand All @@ -102,6 +155,9 @@ internal static unsafe void GetCurrentStackInfo(void* destVector)

i++;
}

// Resize the vector again in case we skipped some frames.
vector->Resize(i);
}
catch (Exception e)
{
Expand All @@ -122,7 +178,10 @@ internal static void GetStackFrameMethodDecl(StackFrame frame, out string method
var sb = new StringBuilder();

if (methodBase is MethodInfo methodInfo)
{
sb.AppendTypeName(methodInfo.ReturnType);
sb.Append(' ');
}

sb.Append(methodBase.DeclaringType?.FullName ?? "<unknown>");
sb.Append('.');
Expand Down
26 changes: 20 additions & 6 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Text;
using Godot.NativeInterop;

Expand Down Expand Up @@ -334,6 +335,21 @@ public static void PrintT(params object[] what)
NativeFuncs.godotsharp_printt(godotStr);
}

[StackTraceHidden]
private static void ErrPrintError(string message, godot_error_handler_type type = godot_error_handler_type.ERR_HANDLER_ERROR)
{
// Skip 1 frame to avoid current method.
raulsntos marked this conversation as resolved.
Show resolved Hide resolved
var stackFrame = DebuggingUtils.GetCurrentStackFrame(skipFrames: 1);
string callerFilePath = ProjectSettings.LocalizePath(stackFrame.GetFileName());
DebuggingUtils.GetStackFrameMethodDecl(stackFrame, out string callerName);
int callerLineNumber = stackFrame.GetFileLineNumber();

using godot_string messageStr = Marshaling.ConvertStringToNative(message);
using godot_string callerNameStr = Marshaling.ConvertStringToNative(callerName);
using godot_string callerFilePathStr = Marshaling.ConvertStringToNative(callerFilePath);
NativeFuncs.godotsharp_err_print_error(callerNameStr, callerFilePathStr, callerLineNumber, messageStr, p_type: type);
}

/// <summary>
/// Pushes an error message to Godot's built-in debugger and to the OS terminal.
///
Expand All @@ -347,8 +363,7 @@ public static void PrintT(params object[] what)
/// <param name="message">Error message.</param>
public static void PushError(string message)
{
using var godotStr = Marshaling.ConvertStringToNative(message);
NativeFuncs.godotsharp_pusherror(godotStr);
ErrPrintError(message);
}

/// <summary>
Expand All @@ -364,7 +379,7 @@ public static void PushError(string message)
/// <param name="what">Arguments that form the error message.</param>
public static void PushError(params object[] what)
{
PushError(AppendPrintParams(what));
ErrPrintError(AppendPrintParams(what));
}

/// <summary>
Expand All @@ -378,8 +393,7 @@ public static void PushError(params object[] what)
/// <param name="message">Warning message.</param>
public static void PushWarning(string message)
{
using var godotStr = Marshaling.ConvertStringToNative(message);
NativeFuncs.godotsharp_pushwarning(godotStr);
ErrPrintError(message, type: godot_error_handler_type.ERR_HANDLER_WARNING);
}

/// <summary>
Expand All @@ -393,7 +407,7 @@ public static void PushWarning(string message)
/// <param name="what">Arguments that form the warning message.</param>
public static void PushWarning(params object[] what)
{
PushWarning(AppendPrintParams(what));
ErrPrintError(AppendPrintParams(what), type: godot_error_handler_type.ERR_HANDLER_WARNING);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ private static void SendToScriptDebugger(Exception e)
}

NativeFuncs.godotsharp_internal_script_debugger_send_error(nFunc, nFile, line,
nErrorMsg, nExcMsg, p_warning: godot_bool.False, stackInfoVector);
nErrorMsg, nExcMsg, godot_error_handler_type.ERR_HANDLER_ERROR, stackInfoVector);
}
}

Expand Down
9 changes: 9 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,4 +1133,13 @@ public readonly unsafe int Size
get => _ptr != null ? *((int*)_ptr - 1) : 0;
}
}

[SuppressMessage("ReSharper", "InconsistentNaming")]
public enum godot_error_handler_type
{
ERR_HANDLER_ERROR = 0,
ERR_HANDLER_WARNING,
ERR_HANDLER_SCRIPT,
ERR_HANDLER_SHADER,
}
}
6 changes: 2 additions & 4 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal static partial void godotsharp_stack_info_vector_destroy(

internal static partial void godotsharp_internal_script_debugger_send_error(in godot_string p_func,
in godot_string p_file, int p_line, in godot_string p_err, in godot_string p_descr,
godot_bool p_warning, in DebuggingUtils.godot_stack_info_vector p_stack_info_vector);
godot_error_handler_type p_type, in DebuggingUtils.godot_stack_info_vector p_stack_info_vector);

internal static partial godot_bool godotsharp_internal_script_debugger_is_active();

Expand Down Expand Up @@ -540,9 +540,7 @@ internal static partial void godotsharp_var_to_bytes(in godot_variant p_what, go

internal static partial void godotsharp_var_to_str(in godot_variant p_var, out godot_string r_ret);

internal static partial void godotsharp_pusherror(in godot_string p_str);

internal static partial void godotsharp_pushwarning(in godot_string p_str);
internal static partial void godotsharp_err_print_error(in godot_string p_function, in godot_string p_file, int p_line, in godot_string p_error, in godot_string p_message = default, godot_bool p_editor_notify = godot_bool.False, godot_error_handler_type p_type = godot_error_handler_type.ERR_HANDLER_ERROR);

// Object

Expand Down
21 changes: 11 additions & 10 deletions modules/mono/glue/runtime_interop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,10 @@ void godotsharp_stack_info_vector_destroy(

void godotsharp_internal_script_debugger_send_error(const String *p_func,
const String *p_file, int32_t p_line, const String *p_err, const String *p_descr,
bool p_warning, const Vector<ScriptLanguage::StackInfo> *p_stack_info_vector) {
ErrorHandlerType p_type, const Vector<ScriptLanguage::StackInfo> *p_stack_info_vector) {
const String file = ProjectSettings::get_singleton()->localize_path(p_file->simplify_path());
EngineDebugger::get_script_debugger()->send_error(*p_func, file, p_line, *p_err, *p_descr,
true, p_warning ? ERR_HANDLER_WARNING : ERR_HANDLER_ERROR, *p_stack_info_vector);
true, p_type, *p_stack_info_vector);
}

bool godotsharp_internal_script_debugger_is_active() {
Expand Down Expand Up @@ -1320,12 +1320,14 @@ void godotsharp_printraw(const godot_string *p_what) {
OS::get_singleton()->print("%s", reinterpret_cast<const String *>(p_what)->utf8().get_data());
}

void godotsharp_pusherror(const godot_string *p_str) {
ERR_PRINT(*reinterpret_cast<const String *>(p_str));
}

void godotsharp_pushwarning(const godot_string *p_str) {
WARN_PRINT(*reinterpret_cast<const String *>(p_str));
void godotsharp_err_print_error(const godot_string *p_function, const godot_string *p_file, int32_t p_line, const godot_string *p_error, const godot_string *p_message, bool p_editor_notify, ErrorHandlerType p_type) {
_err_print_error(
reinterpret_cast<const String *>(p_function)->utf8().get_data(),
reinterpret_cast<const String *>(p_file)->utf8().get_data(),
p_line,
reinterpret_cast<const String *>(p_error)->utf8().get_data(),
reinterpret_cast<const String *>(p_message)->utf8().get_data(),
p_editor_notify, p_type);
}

void godotsharp_var_to_str(const godot_variant *p_var, godot_string *r_ret) {
Expand Down Expand Up @@ -1611,8 +1613,7 @@ static const void *unmanaged_callbacks[]{
(void *)godotsharp_str_to_var,
(void *)godotsharp_var_to_bytes,
(void *)godotsharp_var_to_str,
(void *)godotsharp_pusherror,
(void *)godotsharp_pushwarning,
(void *)godotsharp_err_print_error,
(void *)godotsharp_object_to_string,
};

Expand Down