-
Notifications
You must be signed in to change notification settings - Fork 0
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
op2ext for Outpost 2 v1.4.x #303
base: master
Are you sure you want to change the base?
Conversation
…irectory exported APIs. Added OP2 version check.
Note that I deliberately avoid using exceptions in my code. Outpost 2 was not compiled with exceptions or stack frames, which is a major hindrance for this mechanism. We can't fully avoid exceptions due to the STL (and even if you disable exceptions with /EHsc- and #define _HAS_EXCEPTIONS=0, at least some parts of the STL like filesystem still use them anyway), but we should try to get away from them as much as we can. For now, overhauling error tracking across the project is out of scope for this change. That said, we might consider something along the lines of LLVM's ErrorOr or Google's StatusOr. |
@Arklon1, why is it fundamentally flawed to throw, catch (or not catch as appropriate) exceptions within op2ext or other DLL modules meant to extend an application that doesn't incorporate stack frames? Are boundaries between the primary exe and the dlls not sufficient to allow programming with exceptions in op2ext? Are you advocating for moving away from using std::filesytem in the future due to it throwing exceptions? If so, do you have a preferred library set to use in its stead? -Brett |
@Brett208 Outpost2.exe does not have code for stack frames or stack unwinding, so if a DLL loaded into it throws an exception, it will try to unwind the stack starting from the DLL's memory space until it reaches Outpost2.exe's and blows up. This is one of the many reasons why C++ exceptions are widely panned. I think we should continue to use the STL, despite the complications its use of exceptions brings. (This is one of the major general complaints people tend to have about C++, but there isn't much you can do short of rolling your own STL.) What I am suggesting is that we avoid using them for our own error tracking wherever possible. |
ABI concerns are only really an issue if exceptions cross a module boundary. So long as exceptions are not allowed to escape op2ext and propagate to Outpost2.exe, it won't matter if we use them. I probably won't have time to review the code changes this weekend. |
There's always the possibility of having a bug where you don't catch some exception. A good error handling mechanism should not have inherent instability where it cannot fail gracefully, especially when using a debugger. You could put catch-all blocks everywhere, but that's a hacky way to use exceptions; at that point you might as well use something different anyway. That said, we may actually want to have catch-all blocks around calls to mod DLLs etc. regardless of what we do for error handling elsewhere, since we can't really stop them from throwing exceptions. As for STL usage, a lot of its functions (including those in filesystem) have versions that take a reference to/return std::errc or std::error_code instead of throwing exceptions. We should try to use those where available. |
I disagree with wrapping calls to mods with exception handling code. Such exceptions would be crossing a module boundary, so again, no guarantee of ABI compatibility. It would be a lot of noise without any real benefit. If a module throws an exception across an API boundary, the module is bad, and the module needs to be fixed. If things crash because of a module, it's the module's fault. And honestly, with plugin modules like this, there's no way to guarantee they can't crash and bring down the whole game. I'm fine with catching exceptions we raise ourselves. Anything we should have caught but didn't is a bug, and should be fixed. And it only really makes sense to catch an exception if there is something reasonable you can do to recover from the error. If someone is using the API dead wrong, crashing with a clear error message is an acceptable handling strategy. |
For unhandled exceptions to actually report a useful error and "crash" by the way of std::abort, rather than have a hard ABI crash, I thought the base module's CRT init function/"real" entry point needs to be compiled with code to handle them? I wasn't going to suggest we do anything with exceptions caught from mods other than report an error and then assert or abort (obviously there's no way we can recover), since there is no built-in unhandled exception path to propagate them to - in other words, we implement the "unhandled exception" handler ourselves explicitly. But after posting my comment yesterday, I realized that different CRTs across modules can also cause ABI incompatibilities with exceptions, even if they were compiled with the same compiler and exception model. So that'd greatly diminish the usefulness of having that anyway. Not to mention, even if Outpost2.exe did happen to be compiled with exception support, propagating exceptions up to it would still likely not work. I still consider the practical inability to report errors across module boundaries to be a major deficiency of C++ exceptions, and since op2ext is a DLL loaded into an app we have no way of making ABI-compatible, we are inherently invoking this dilemma. It goes without saying that uncaught exceptions would have to be considered bugs to be fixed. My issue with it is any time it's used presents an opportunity to introduce such bugs, the result of which is an ABI-related crash that can look bizarre. The equivalent bug with the error code idiom, i.e. forgetting to check for an error, instead leads to (now possibly unsafe) code paths not being skipped over, which can either crash, return the error as-is, or overwrite/discard it and maybe return some other success/error. Crashing is bad, but the code locality will often enough be reasonably close. Returning the error as-is would be best-case scenario, since then other code that does properly check for non-success should treat it as if the callee failed anyway, although it could have had side effects (not that that matters if it's treated as fatal). Overwriting/discarding it tends to lead to either crashing or returning another error downstream in practice, which often still has reasonable code locality, although it could look like success with undefined side effects which would be the worst case to debug. I would rather debug most of those scenarios than a failed stack unwind the debugger may or may not be able to reconstruct the call stack from. The other thing with error codes is we could make use of [[nodiscard]] on the error code type itself and/or functions returning it, as StatusOr does, to have the compiler help enforce error checking, which does a great job at avoiding these bugs. Not that it does anything for us in the present, I did stumble upon this interesting proposal for static exceptions: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0709r4.pdf While the biggest justifications the paper spends time on are related to resolving performance issues with the current model, it also mentions being more ABI-safe (although maybe not safe enough for our use case as it does involve extending the ABI and calling conventions). It'd be nice if the ISO committee would ever approve of some major overhaul of exceptions, because it's very badly needed. |
I would rather debug a crash from an unhandled exception than an error return value being silently ignored or improperly handled. At least with the unhandled exception, you're most likely going to know that something is wrong. The other case can easily go missed for years. I have read discussions about improving exceptions in C++. One of the concerns was that people insisting on disabling or not using exceptions was causing a split in the language, and an inability for mix code from either model. I would rather just use exceptions. If done correctly, the code can be much easier to read. And if used incorrectly, it usually becomes obvious pretty fast. |
Apologies for the super long winded response; I'll first preface this by saying (professionally) I work on a pretty large codebase that bans exceptions with the exception (heh) of third party libraries and feel like I have seen enough of their pitfalls in the places we do have to interface with code that does use exceptions. I'm also firmly in the "we shouldn't use exceptions" camp along with Arklon. I think it's a fundamentally broken C++ language feature ignoring the ABI compatibility aspect for a moment. If C++ exceptions worked like exceptions in most other high level languages then perhaps this would be a different story. I'd argue exceptions offer the exact opposite of easy-to-debug, maintainable code. There's no static enforcement that exceptions are actually caught like in other languages; if we add code that uses From a cyclomatic complexity standpoint, exceptions make code harder to understand (since now we have to assume that anything we do in a particular function can change control flow unexpectedly by throwing, and since this is c++ not just function calls but any sort of expression that could involve operator overloads). It requires proper use of RAII to ensure that program state doesn't get corrupted, we don't leak memory, etc. by abruptly leaving a block of code. (Yes, I realize some of these arguments could equally apply to other languages. However, in almost every language I can think of that sanely implements exceptions, the exception machinery provides things such as a stacktrace from the point where the exception was thrown, stronger enforcement that functions declare which exceptions are allowed to be thrown out from them, more consistent cleanup for heap-allocated memory, objects in an indeterminate state, etc (which is usually more a result of the memory-safety aspects of said language than anything to do with exceptions proper)). Beyond that, I think there are plenty of arguments to be made against exceptions in the name of possible ABI incompatibilities:
I think the issue of "exceptions force you to deal with the issue" is moot, we can use things like
I agree, but we can also avoid handing authors of other modules a metaphorical rope to hang themselves with so to speak. If in the theoretical future the ISO C++ standards committee does its thing and comes up with a way to improve exceptions then perhaps we can and should reevaluate. |
I understand there has traditionally been a lot of push back against exceptions in C++ code. Though I feel a lot of those concerns applied more to pre-c++17 code. I would urge people to re-evaluate the current situation. Writing exception safe resource handling code, using RAII, is now pretty much the norm. Ever since the introduction of Cyclomatic complexity between exception handling versus error code returns may or may not change. The main difference seems to be that error return codes forces explicit handling so that complexity is very evident to the user. That might make it a little easier to reason about in some contexts, though forcing programmers to always write such code is not exactly very friendly. At any rate, high cyclomatic complexity seems to be a symptom of bad code, more so than exceptions versus error return codes. ABI concerns relating to exceptions shouldn't be an issue because exceptions shouldn't be crossing module boundaries. If no exceptions cross a module boundary, the ABI is never relevant. And as stated, modules can be written with any compiler, which may not have a compatible ABI, and hence why we should never allow exceptions to cross a module boundary. That's the whole point of the restriction. So debates about how to handle exceptions if they cross a module boundary are kind of irrelevant. And despite the use of patching, module boundaries really aren't all that vague if you really stop and think about it. How are patched hook calls really all that different from a normal call across a module boundary? And it's not like we've ever written code patches that explicitly raise an exception in another module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started a partial review.
I only really glanced at the failing unit tests. I assume that relates to the exception discussion.
Would be nice if that first commit had been broken down into smaller chunks. It's quite a chore to review that much in a single commit, and there isn't much in the way of commit comments to explain each change.
srcDLL/DllMain.cpp
Outdated
if ((dwReason == DLL_PROCESS_ATTACH) && CommandOptionExists("OPU")) { | ||
wchar_t filename[MAX_PATH] = L""; | ||
GetModuleFileNameW(NULL, &filename[0], MAX_PATH); | ||
|
||
if (fs::path(filename).filename() == "Outpost2.exe") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the extra checks added here? I think I understand why the CommandOptionExists("OPU")
check was added, based on what I've been told of the new architecture for 1.4.0. I'm less certain why the module filename check was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The winmm.dll shim (that loads op2ext.dll as an import) could potentially be loaded into any process, since it is a system DLL. This is just out of an abundance of caution.
srcDLL/DllMain.cpp
Outdated
// Redirects Outpost2.ini to be accessed from the OPU directory. | ||
void RedirectIniFile() | ||
{ | ||
const auto iniPath = GetOutpost2IniPath(); | ||
Op2MemCopy(0x00547090, iniPath.length() + 1, iniPath.data()); // Overwrite gConfigFile.iniPath | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this copy be bounds checked? I assume the destination buffer is statically sized. If it's guaranteed to fit, we should probably add a note as to why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The destination buffer is MAX_PATH-sized. While it could be bounds-checked, if we're trying a path longer than MAX_PATH, we're already going to explode due to OS limits. (Win10 did add large path support, but apps need to opt-in to it via the manifest and be coded to use them, and there's no real chance of doing that for OP2.)
srcStatic/ConsoleArgumentParser.cpp
Outdated
static CommandIterator FindCommandSwitch(CommandIterator begin, CommandIterator end, const std::string& option) | ||
{ | ||
return std::find_if(begin, end, [&option](const std::string& argument) | ||
{ return ((argument[0] == '/') || (argument[0] == '-')) && (_stricmp(&argument[1], option.data()) == 0); }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't bounds check that argument
contains at least 1 character before accessing it.
auto it = FindCommandSwitch(begin, end, option); | ||
return ((it != end) && (++it != end)) ? it : end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could eliminate the check for (++it != end)
, since end
is the default return value when no switch is found:
return (it != end) ? ++it : end;
throw std::runtime_error(message); | ||
std::vector<std::string> directories; | ||
|
||
for (auto it = arguments.cbegin(); (it = GetCommandOption(it, arguments.cend(), "loadmod")) != arguments.cend();) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of a minor point, though this for
loop expression is a bit unusual and hard to read. Might be nice if a range-for loop could have been used:
for (const auto& argument : GetCommandLineOptions(arguments, "loadmod"))
Could be done with either a std::vector
return type, or a range return type. Not sure it's worth it to create a range return type though.
srcStatic/GameModules/IniModule.cpp
Outdated
@@ -9,7 +10,10 @@ IniModule::IniModule(IniSection iniSection) | |||
{ | |||
try { | |||
// Get the DLL name from the corresponding section | |||
LoadModuleDll(DllName()); | |||
const fs::path path = DllName(); | |||
if (path.empty() == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!path.empty())
LoadModuleDll(DllName()); | ||
const fs::path path = DllName(); | ||
if (path.empty() == false) { | ||
LoadModuleDll((path.is_absolute() ? path : GetOpuDirectory() / path).string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like maybe there is already a method to add prefixes to relative paths, while leaving absolute paths as is. Though perhaps that's part of OP2Utility, rather than op2ext. @Brett208, you want to comment on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanRStevens, I'm not sure. There is a AppendSubDirectory
in OP2Utility, although I'm not sure it is a well thought out method that I wrote. I think this line of code is fairly clear in intent without modification.
I think the important thing here is to make sure the current environment directory isn't used, especially if using a debugger because it may not be coincident with the actual Outpost 2 directory. And I think this has been taken into account.
if (IsModuleRequested(sectionName, "IPDropDown")) { | ||
if (IsModuleRequested(sectionName, "IPDropDown", true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean parameters are not very self documenting at the call site. Sometimes it makes more sense to have two different functions, so the function names can explain the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This boolean argument added to IsModuleRequested represents the default return if the iniFile does not contain an entry for a built in module. Implementation before 3.1.0 would not load a built in module if it was not present in the ini file. 3.1.0 adds the ability to set a default for each module whether it should load or not load if not present in the ini file.
This is causing some of the unit test failures. Several unit tests had assumed IP Drop Down would not load if not present in a blank ini file which is not true in 3.1.0. Should be easy to account for in the unit tests.
If a change were made to improve readability of this source code line, it might be easier to use an enum class to be more descriptive of what the boolean meant than just passing a boolean. I would probably prefer this to multiple functions in this case but probably more of a personal preference. I think there is a desire to remove the concept of built in modules in the future though.
#include <windows.h> // Cannot use WIN32_LEAN_AND_MEAN (it does not contain CommandLineToArgvW) | ||
#define WIN32_LEAN_AND_MEAN | ||
#include <windows.h> | ||
#include <shellapi.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this change could use a bit of documentation or reference material, at least as a PR comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommandLineToArgvW is defined in shellapi.h. It's more efficient to just include that one header than unleash everything gated by undef'ing WIN32_LEAN_AND_MEAN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to official documentation to confirm this:
https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw
srcStatic/GameModules/IniModule.cpp
Outdated
std::string IniModule::Directory() | ||
{ | ||
auto dllSetting = DllName(); | ||
return fs::path(dllSetting).remove_filename().string(); | ||
fs::path iniSetting(iniSection.GetValue("Path")); | ||
|
||
if (iniSetting.empty()) { | ||
iniSetting = DllName(); | ||
iniSetting.remove_filename(); | ||
} | ||
|
||
return iniSetting.string(); | ||
} | ||
|
||
std::string IniModule::DllName() | ||
{ | ||
auto defaultDllName = (fs::path(Name()) / "op2mod.dll").string(); | ||
return iniSection.GetValue("Dll", defaultDllName); | ||
return iniSection.GetValue("Dll"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too sure why this part is being changed. I may be getting too tired to review though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to facilitate non-DLL mods, e.g. language mods. Such mods can either specify e.g. Path = lang/it
under the mod's ini section, or as
[ExternalModules]
lang/it = yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we already had support for non-DLL mods. In particular, resource mods. Though perhaps support was asymmetric for the different mod loading methods. This may bear closer scrutiny.
Two problems were noted: 1. std::filesystem::create_directory was trying to create a directory and its parent directory, which is not allowed. The parent directory must be created first. This was caused by adding the OPU directory inbetween the Outpost 2 directory and console module's directory. 2. I had poorly written the test in the past to assume that only 1 module would register. 3.1.0 is causing the IP Drop Down module to also be registered as a module. I rewrote the test to assume another module could be present. There isn't a simple way to pull a module's index after registration, which could make reading this test easier.
@Arklon1, I commited a change to fix one of the failing unit tests. If that is not ok with you, please force delete the commit. I can poke around at resolving some of the other failures as well if it doesn't bother you. I suspect several of the other failures are very similar to what I just fixed. In regards to using exceptions, I come from more of an application development background using more of a managed language like C# where exceptions are pretty much standard. So I am more comfortable writing them then using error codes (although I wouldn't claim to be good at writing quality exception statements). I will defer to everyone else on this thread with more C/C++ experience whether it is a wise idea or not to use exceptions in op2ext and will attempt to adjust my contributions as appropriate. I probably introduced the bulk of them. I noticed earlier in the thread Arklon recommended looking at Google StatusOr which is a child of the Abseil project. I've briefly looked at Abseil in the past, and it looked like a very large library that covered many topics. Is there an accepted way to bring one component of the library as a dependency, or do people typically link against the entire project? Last question for now, @Arklon1, did you end up moving the IP Drop Down patch into OPUPatch? If so, I would propose we delete it from op2ext so it doesn't exist in two places. I thought you had mentioned wanting to move it over to OPUPatch in separate correspondence, but maybe I'm remembering wrong. |
Fixing the tests is fine. I was hoping to get time to do that over the weekend, but I've been busier than expected. Rolling our own ErrorOr implementation or separating out ErrorOr/StatusOr probably wouldn't be too awful. There is also this: https://github.com/TartanLlama/expected Even just using std::pair<ReturnType, ErrorCodeType> is pretty comfortable now with C++17 structured bindings, but a custom type would be much preferred, particularly so we could take advantage of the The IP drop down has not been moved to OPUPatch, although I plan to do so at some point. I also plan to move most of the file search path-related code from OPUPatch into op2ext in the future, which would entail integrating Patcher and Capstone into op2ext. |
So the C ISO committee actually did recently approve of a very similar static exceptions proposal to the one I linked earlier: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1095r0.pdf |
So I keep putting off review of this PR. It's pretty massive, and I don't typically have the concentration nor desire to sit through a full review session. Could this please be reworked into smaller commits? I would even welcome separate PRs for different sections of it. I think breaking it up would help in a few ways. In particular, having a smaller section to review, with tests still passing, would make it easier to merge changes. Smaller commits would also provide more opportunity for explanatory git commit messages as to why certain changes were made, or why they are correct. Some of those decisions are probably better documented in git commit messages, where they would follow a project around if it changed hosting, whereas PR comments might be lost. |
Allows creating directories recursively if needed. The OPU directory may not be present when unit tests begin executing.
The minimum number of modules loaded is non-deterministic now that built in modules can be loaded even if they are not present in the ini file
op2ext will no longer log a message for missing dll modules unless a dll path is specifically called out in the ini file
Changes for op2ext v3.1.0: