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.

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

[3.x] SCons: Disable C++ exception handling #80622

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 11 additions & 2 deletions SConstruct
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ opts.Add(EnumVariable("lto", "Link-time optimization (production builds)", "none
opts.Add(BoolVariable("deprecated", "Enable deprecated features", True))
opts.Add(BoolVariable("minizip", "Enable ZIP archive support using minizip", True))
opts.Add(BoolVariable("xaudio2", "Enable the XAudio2 audio driver", False))
opts.Add(BoolVariable("disable_exceptions", "Force disabling exception handling code", True))
opts.Add("custom_modules", "A list of comma-separated directory paths containing custom modules to build.", "")
opts.Add(BoolVariable("custom_modules_recursive", "Detect custom modules recursively for each specified path.", True))

Expand Down Expand Up @@ -480,6 +481,16 @@ if selected_platform in platform_list:
print(" Please adjust your scripts accordingly.")
Exit(255)

# Disable exception handling. Godot doesn't use exceptions anywhere, and this
# saves around 20% of binary size and very significant build time (GH-80513).
if env["disable_exceptions"]:
if env.msvc:
env.Append(CPPDEFINES=[("_HAS_EXCEPTIONS", 0)])
else:
env.Append(CCFLAGS=["-fno-exceptions"])
elif env.msvc:
env.Append(CCFLAGS=["/EHsc"])

# Configure compiler warnings
if env.msvc: # MSVC
# Truncations, narrowing conversions, signed/unsigned comparisons...
Expand All @@ -492,8 +503,6 @@ if selected_platform in platform_list:
env.Append(CCFLAGS=["/W2"] + disable_nonessential_warnings)
else: # 'no'
env.Append(CCFLAGS=["/w"])
# Set exception handling model to avoid warnings caused by Windows system headers.
env.Append(CCFLAGS=["/EHsc"])

if env["werror"]:
env.Append(CCFLAGS=["/WX"])
Expand Down
2 changes: 1 addition & 1 deletion editor/plugins/script_editor_plugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ static bool _is_built_in_script(Script *p_script) {

class EditorScriptCodeCompletionCache : public ScriptCodeCompletionCache {
struct Cache {
uint64_t time_loaded;
uint64_t time_loaded = 0;
RES cache;
};

Expand Down
9 changes: 9 additions & 0 deletions modules/denoise/SCsub
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ env_oidn.AppendUnique(CPPDEFINES=["NDEBUG"]) # No assert() even in debug builds

env_thirdparty = env_oidn.Clone()
env_thirdparty.disable_warnings()

if env["disable_exceptions"]:
# OIDN hard-requires exceptions, so we re-enable them here.
if env.msvc and ("_HAS_EXCEPTIONS", 0) in env_thirdparty["CPPDEFINES"]:
env_thirdparty["CPPDEFINES"].remove(("_HAS_EXCEPTIONS", 0))
env_thirdparty.AppendUnique(CCFLAGS=["/EHsc"])
elif not env.msvc and "-fno-exceptions" in env_thirdparty["CCFLAGS"]:
env_thirdparty["CCFLAGS"].remove("-fno-exceptions")

env_thirdparty.add_source_files(thirdparty_obj, thirdparty_sources)
env.modules_sources += thirdparty_obj

Expand Down
9 changes: 3 additions & 6 deletions modules/fbx/fbx_parser/FBXMaterial.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,8 @@ Video::Video(uint64_t id, const ElementPtr element, const Document &doc, const s

if (Content && !Content->Tokens().empty()) {
//this field is omitted when the embedded texture is already loaded, let's ignore if it's not found
Copy link
Member Author

@akien-mga akien-mga Aug 14, 2023

Choose a reason for hiding this comment

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

@RevoluPowered We shouldn't use exceptions in Godot code, how can I rewrite this to handle the failure case gracefully? (Current diff just removes try { and } catch(...) { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to be straight from Assimp where it's still using this hack:
https://github.com/assimp/assimp/blob/556c89b5f143738720e17572faa8e272d4eec380/code/AssetLib/FBX/FBXMaterial.cpp#L304-L364

I don't see it intentionally throwing and catching exceptions, so I guess this code will just crash and instead of adding proper failure handling they just wrapped it in exceptions... sigh

So what can crash here? token == nullptr at least I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just changed it to replace the try { } catch (...) { } by if (token), assuming that if it's failing it may return a nullptr. Add added another ERR_FAIL_COND, there was already one, it probably leaves things in a broken state if the condition is met but I'm not sure we even use the FBX Video class at all, so I don't want to do a deep dive.

try {
const Token *token = GetRequiredToken(Content, 0);
const Token *token = GetRequiredToken(Content, 0);
if (token) {
const char *data = token->begin();
if (!token->IsBinary()) {
if (*data != '"') {
Expand All @@ -341,6 +341,7 @@ Video::Video(uint64_t id, const ElementPtr element, const Document &doc, const s
// First time compute size (it could be large like 64Gb and it is good to allocate it once)
for (uint32_t tokenIdx = 0; tokenIdx < numTokens; ++tokenIdx) {
const Token *dataToken = GetRequiredToken(Content, tokenIdx);
ERR_FAIL_COND(!dataToken);
size_t tokenLength = dataToken->end() - dataToken->begin() - 2; // ignore double quotes
const char *base64data = dataToken->begin() + 1;
const size_t outLength = Util::ComputeDecodedSizeBase64(base64data, tokenLength);
Expand Down Expand Up @@ -383,10 +384,6 @@ Video::Video(uint64_t id, const ElementPtr element, const Document &doc, const s
content = new uint8_t[len];
::memcpy(content, data + 5, len);
}
} catch (...) {
// //we don't need the content data for contents that has already been loaded
// ASSIMP_LOG_VERBOSE_DEBUG_F("Caught exception in FBXMaterial (likely because content was already loaded): ",
// runtimeError.what());
}
}

Expand Down
4 changes: 2 additions & 2 deletions platform/android/detect.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,11 @@ def configure(env):
env["RANLIB"] = compiler_path + "/llvm-ranlib"
env["AS"] = compiler_path + "/clang"

# Disable exceptions and rtti on non-tools (template) builds
# Disable rtti on non-tools (template) builds.
if env["tools"]:
env.Append(CXXFLAGS=["-frtti"])
else:
env.Append(CXXFLAGS=["-fno-rtti", "-fno-exceptions"])
env.Append(CXXFLAGS=["-fno-rtti"])
# Don't use dynamic_cast, necessary with no-rtti.
env.Append(CPPDEFINES=["NO_SAFE_CAST"])

Expand Down
8 changes: 0 additions & 8 deletions platform/iphone/detect.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ def get_opts():
),
("IPHONESDK", "Path to the iPhone SDK", ""),
BoolVariable("ios_simulator", "Build for iOS Simulator", False),
BoolVariable("ios_exceptions", "Enable exceptions", False),
("ios_triple", "Triple for ios toolchain", ""),
]

Expand Down Expand Up @@ -157,13 +156,6 @@ def configure(env):
env.Append(CPPDEFINES=["NEED_LONG_INT"])
env.Append(CPPDEFINES=["LIBYUV_DISABLE_NEON"])

# Disable exceptions on non-tools (template) builds
if not env["tools"]:
if env["ios_exceptions"]:
env.Append(CCFLAGS=["-fexceptions"])
else:
env.Append(CCFLAGS=["-fno-exceptions"])

# Temp fix for ABS/MAX/MIN macros in iPhone SDK blocking compilation
env.Append(CCFLAGS=["-Wno-ambiguous-macro"])

Expand Down
6 changes: 3 additions & 3 deletions platform/javascript/detect.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ def configure(env):
print('Note: Forcing "initial_memory=64" as it is required for the web editor.')
env["initial_memory"] = 64
else:
# Disable exceptions and rtti on non-tools (template) builds
# These flags help keep the file size down.
env.Append(CCFLAGS=["-fno-exceptions", "-fno-rtti"])
# Disable rtti on non-tools (template) builds.
# This helps keep the file size down.
env.Append(CCFLAGS=["-fno-rtti"])
# Don't use dynamic_cast, necessary with no-rtti.
env.Append(CPPDEFINES=["NO_SAFE_CAST"])

Expand Down
4 changes: 2 additions & 2 deletions servers/audio_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,9 @@ class AudioServer : public Object {

struct Effect {
Ref<AudioEffect> effect;
bool enabled;
bool enabled = false;
#ifdef DEBUG_ENABLED
uint64_t prof_time;
uint64_t prof_time = 0;
#endif
};

Expand Down