Fix argvA initialization for wxGTK under Windows (PR #24659)
Ryan Ogurek
Update conditional directives to enable argvA if !WXMSW instead of if !WINDOWS (It seems originally there was
confusion/misunderstanding of the roles of the two macros as toolkit and platform indicators respectively)
Fix argvA not being initialized in InitIfNecessary due to argc check returning early.
See issue #24611
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24659
Commit Summary
- 0e08dc5 Fix argvA initialization for wxGTK under Windows
File Changes
(2 files)
- M include/wx/private/init.h (12)
- M src/common/init.cpp (45)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24659.patch
- https://github.com/wxWidgets/wxWidgets/pull/24659.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Sorry, I feel uncomfortable about this because I don't understand what's going on here any longer: how can it happen that argc != 0 but argvA == 0 (when it's used)? I think this shouldn't happen, i.e. we should respect the invariant argv == 0 || argvA != 0.
Am I missing something?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
To be honest this doesn't make sense to me either. However in testing when I had things the original way (with the original check and the argvA fill after that), the program would immediately crash with a nullptr dereference from trying to use argvA.
I did some printf debugging and found out that the Initialize function wasn't ever run (at least within the time from start to crash) and that InitIfNecessary was run, but the the check caused it to return before actually doing anything.
I did some grep -ring to find anywhere and everywhere argvA was used, so I don't think I missed enabling some other routine that should've filled it...
we should respect the invariant
argv == 0 || argvA != 0
I guess I don't understand... are you suggesting this check should be used instead?
To me, InitIfNecessary's job seems to be to initialize these variables if they haven't been already, so checking if argvA is nullptr (and thus not initialized) and then initializing it if so seems to be expected behavior.
In the case of argvA, in an initialized state, it shouldn't ever be nullptr right? Because at the very least argvA[0] should be executable? At this point perhaps I don't understand how (if at all) wxWidgets should have processed the arguments, and maybe that's where I'm confused and/or wrong in my logic.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
To be honest this doesn't make sense to me either. However in testing when I had things the original way (with the original check and the argvA fill after that), the program would immediately crash with a nullptr dereference from trying to use argvA.
When this happened, was argv already initialized? If it was, we need to find the place which initialized it and initialize argvA there too.
I did some printf debugging and found out that the Initialize function wasn't ever run (at least within the time from start to crash) and that InitIfNecessary was run, but the the check caused it to return before actually doing anything.
But if it didn't do anything, it must mean that argc != 0, so it must have been set somewhere?
we should respect the invariant
argv == 0 || argvA != 0I guess I don't understand... are you suggesting this check should be used instead?
No, sorry for the confusion, this is not a check, but the invariant, i.e., in words, we should always have valid argvA (when it's used at all, i.e. not when using wxMSW) if we have valid argv.
In the case of argvA, in an initialized state, it shouldn't ever be nullptr right? Because at the very least argvA[0] should be executable? At this point perhaps I don't understand how (if at all) wxWidgets should have processed the arguments, and maybe that's where I'm confused and/or wrong in my logic.
It's possible that someone calls wxInitialize(0,0) (well, with references instead of values, but this is not the point), so we need to handle this case by allocating 1-element argv and argvA arrays containing a single null entry (not even the executable name, because this would correspond to argc == 1). However if the real entry point (i.e. main()) is called, we should have argc >= 1.
I guess the problem is that we use argc != 0 as the test for checking whether we were initialized, but we should use argv != nullptr instead.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
To be honest this doesn't make sense to me either. However in testing when I had things the original way (with the original check and the argvA fill after that), the program would immediately crash with a nullptr dereference from trying to use argvA.
When this happened, was
argvalready initialized? If it was, we need to find the place which initialized it and initializeargvAthere too.
I didn't think to check. Give me a moment and I can see.
I did some printf debugging and found out that the Initialize function wasn't ever run (at least within the time from start to crash) and that InitIfNecessary was run, but the the check caused it to return before actually doing anything.
But if it didn't do anything, it must mean that
argc != 0, so it must have been set somewhere?
Right, that makes sense. I can try and look...
this is not a check, but the invariant, i.e., in words, we should always have valid
argvA(when it's used at all, i.e. not when using wxMSW) if we have validargv.
Ah, ok I see. It seems then I've learned a new word/concept today, thanks! :)
It's possible that someone calls
wxInitialize(0,0)(well, with references instead of values, but this is not the point), so we need to handle this case by allocating 1-elementargvandargvAarrays containing a single null entry (not even the executable name, because this would correspond toargc == 1). However if the real entry point (i.e.main()) is called, we should haveargc >= 1.
Shouldn't that still be argc == 0 since the array is simply null-terminated, but there's no actual data in it? Or is that what you mean, that adding the executable name would then be argc == 1, but 1-length array w/ a nullptr element would be argc == 0?
I guess the problem is that we use
argc != 0as the test for checking whether we were initialized, but we should useargv != nullptrinstead.
Would then we want if ( argv || !argvIn ) to be the check? I actually had this thought myself when I was looking at the code that it was slightly odd that argc was checked when in both principal and in practice, it was argv that mattered.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Update:
In the gtk app init, where argvA is used, and without the rearranging for InitIfNecessary, at that time argvA is nullptr, and argv is valid.
So argv is being initialized somewhere argvA isn't.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Interestingly it seems as though wxInitData::Initialize is never run...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
in src/msw/main.cpp I've traced the wxEntry that is used on windows (compatible with WinMain), and it calls wxInitData::Get() to pass argc and argv to wxEntry. At this point the WinMain wxEntry has called wxMSWEntryCommon() which calls wxInitData().Get().MSWInitialize().
MSWInitialize() sets argvMSW and... ding ding ding argv.
So, wxInitData::Initialize() indeed never is called...
So that means that this block here:
#ifndef __WXMSW__ argvA = argvIn; #endif
Was an oversight. It doesn't do anything because it's never called. Even if it was, the assert above (rightly) would cause issues because it would mean argv is attempting to be reinitialized.
This makes the structure a bit convoluted. It seems wxInitData::Initialize() (and that little argvA = argvIn) is used under other platforms, so that can't be removed, but the preprocessor conditional also cannot be removed, because of course with wxMSW argvA doesn't exist.
It would seem then that the best thing to do is generate argvA in MSWInitialize with a conditional #ifndef __WXMSW__, using the same routine as is in InitIfNecessary, but also keep the block that is in InitIfNecessary so it fulfills its job should there be an edge case that requires it (and then it will initialize argv and argvA, but argvMSW will be nulled. Whether that's correct I'm unsure).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
@ryancog pushed 1 commit.
- de93000 Revert InitIfNecessary argvA change, fill argvA in wxInitData::MSWInitialize()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> +#ifndef __WXMSW__
+ // We need to convert from wide arguments back to the narrow ones.
+ argvA = new char*[argc + 1];
+ argvA[argc] = nullptr;
+
+ ownsArgvA = true;
+
+ for ( int i = 0; i < argc; i++ )
+ {
+ // Try to use the current encoding, but if it fails, it's better to
+ // fall back to UTF-8 than lose an argument entirely.
+ argvA[i] = wxConvWhateverWorks.cWC2MB(argv[i]).release();
+ }
+#endif // !__WXMSW__
Now the logic indeed makes sense to me, thanks!
But this code seems to exactly duplicate the code below, so what about extracting it into InitArgsA() and calling it from both places instead of copying it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
@ryancog commented on this pull request.
> +#ifndef __WXMSW__
+ // We need to convert from wide arguments back to the narrow ones.
+ argvA = new char*[argc + 1];
+ argvA[argc] = nullptr;
+
+ ownsArgvA = true;
+
+ for ( int i = 0; i < argc; i++ )
+ {
+ // Try to use the current encoding, but if it fails, it's better to
+ // fall back to UTF-8 than lose an argument entirely.
+ argvA[i] = wxConvWhateverWorks.cWC2MB(argv[i]).release();
+ }
+#endif // !__WXMSW__
But this code seems to exactly duplicate the code below, so what about extracting it into
InitArgsA()and calling it from both places instead of copying it?
Makes sense to me, will do!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
@ryancog commented on this pull request.
> +#ifndef __WXMSW__
+ // We need to convert from wide arguments back to the narrow ones.
+ argvA = new char*[argc + 1];
+ argvA[argc] = nullptr;
+
+ ownsArgvA = true;
+
+ for ( int i = 0; i < argc; i++ )
+ {
+ // Try to use the current encoding, but if it fails, it's better to
+ // fall back to UTF-8 than lose an argument entirely.
+ argvA[i] = wxConvWhateverWorks.cWC2MB(argv[i]).release();
+ }
+#endif // !__WXMSW__
Would you prefer it be a helper function (taking in the values of argc and argv (well, the pointer at least) and a reference to argvA and building it) or a member function of wxInitData?
I'm not sure what the philosophy is here. Since the include is already in a private subdirectory, it seems fine to add helpers as members there?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> +#ifndef __WXMSW__
+ // We need to convert from wide arguments back to the narrow ones.
+ argvA = new char*[argc + 1];
+ argvA[argc] = nullptr;
+
+ ownsArgvA = true;
+
+ for ( int i = 0; i < argc; i++ )
+ {
+ // Try to use the current encoding, but if it fails, it's better to
+ // fall back to UTF-8 than lose an argument entirely.
+ argvA[i] = wxConvWhateverWorks.cWC2MB(argv[i]).release();
+ }
+#endif // !__WXMSW__
Yes, it's definitely fine to add a member function to do it, TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
@ryancog pushed 1 commit.
- 57d7a9b Added wxInitData::BuildArgvA() to avoid code duplication
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Ok, I've created wxInitData::BuildArgvA(), guarded it with #ifndef __WXMSW__ and used it in InitIfNecessary() and MSWInitialize()
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Hmm... it seems one of the CI tests failed... but one that failed claims "success" at the end (so clearly I don't know how to read the log.) and the other one fails to a test related to a completely unrelated GUI element? I'm not sure how I would've managed to break that...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Must be an AppVeyor fluke, this is clearly unrelated. I've relaunched this build just to check.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Gotcha. Assuming that that completes without issue, I think that's everything then. Been playing with the builds a bit as I work on my application and I haven't run into any issues. knock on wood
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Weird. It still failed, albeit somewhere else and still seems to be unrelated.
running under Windows Server 2019 (build 17763), 64-bit edition as appveyor Could Windows Server be an issue? (Since they're GUI calculation test cases that seem to be failing?)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Well the rest of the tests just died on their own because they timed out…
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I'm finally merging this with just some minor renaming, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Awesome!
Fwiw I named it BuildArgvA instead of InitArgvA because there's technically the case where argvA is filled by a char** argv. I don't recall exactly, but I know one of the wxEntry's (or one of the init functions?) did that.
So build made more sense to me since it differentiated those two things... it's still init'ing argvA, but via the process of building it from a wchar.
Not something that really matters that much I figure, but I thought I'd explain why I named it that originally :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, I agree that this explanation makes sense, but naming all of the functions InitSomething seemed appealing from consistency point of view as well...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
Understandable :)
I think either way it's plenty logical.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.