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

Fix String initialization in GCC 4.8.5 #1977

Merged
merged 3 commits into from
Nov 21, 2019

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Nov 20, 2019

This PR fixes the instablity problems related to the String class since #1951. The problem is present only in Linux with GCC 4.8.5, which silently ignores the union intializer and member variables are left un-initialised at construction time.

This is the initializer which causes the problem:

	union {
		struct {
			size_t u32[STRING_OBJECT_SIZE / 4] = {0};
		};
		PtrBuf ptr;
		SsoBuf sso;
	};

Simplifying it has no effect:

	union {
		PtrBuf ptr = { nullptr, 0, 0 };
		SsoBuf sso;
	};

So it seems something to be avoided generally.

The solution, therefore, is to add a default constructor which does the initialisation and ensure it's used by all the other constructors.

This has been tested using HostTests build and flashed in a Linux VM.

Note that this PR also addresses another GCC 4.8.5 annoyance in its limited ability to handle constexpr functions. This means the HostTests can be built in Linux.

@aemseemann
Copy link
Contributor

aemseemann commented Nov 20, 2019

This is exactly the issue mentioned in #1817. (BTW: I can't express enough my appreciation for the gdbstub integration, which was incredible helpful in tracking this down.)

However, may I suggest to take care of another minor issue within this PR:
In #1951, the following overload was introduced:

int indexOf(const char* s2_buf, size_t fromIndex = 0, size_t s2_len = 0)  const;

This breaks common patterns like haystack.indexOf("needle"), because of the default argument s2_len = 0, i.e. the search string is truncated to an empty string which is always found at position 0. Before SSO, a const char * argument was implicitly converted to a String and correctly handled by the indexOf(const String&, ...)` overload.

My suggestion would be something like this:

int indexOf(const char* s2_buf, size_t fromIndex, size_t s2_len)  const;
int indexOf(const char* s2_buf, size_t fromIndex = 0)  const
{
    return indexOf(s2_buf, fromIndex, strlen(s2_buf));
}

@slaff slaff added this to the 4.0.1 milestone Nov 20, 2019
@slaff
Copy link
Contributor

slaff commented Nov 20, 2019

This PR fixes the instability problems related to the String class since #1951

Tested and confirmed to do the work. @mikee47 when you have time check @aemseemann comment.

@mikee47
Copy link
Contributor Author

mikee47 commented Nov 20, 2019

@aemseemann Yes, I agree yours is a much better way to handle this - thanks for pointing it out. I'll make the change.

@slaff slaff removed the 3 - Review label Nov 21, 2019
@slaff slaff merged commit dd07ef9 into SmingHub:develop Nov 21, 2019
@slaff
Copy link
Contributor

slaff commented Nov 21, 2019

@mikee47 Thanks a lot for your help!

@mikee47
Copy link
Contributor Author

mikee47 commented Nov 21, 2019

I broke it so I should fix it!

@mikee47 mikee47 deleted the fix/String-uninitialised branch November 21, 2019 08:10
@etmmahi
Copy link
Contributor

etmmahi commented Nov 21, 2019

Thanks for the fix, my 'develop' code is working again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants