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

[C++ification] Take care of a few warnings and don't use LTO #3873

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

grendello
Copy link
Contributor

Desktop compilers mis-detect memory allocation size in our bundled property
code if fortified version of strncpy is used (even though the size
calculation is correct). Use memcpy instead.

LTO doesn't appear to be ready for use just yet. It gives warnings similar to
these:

jni-wrappers.hh(143,41): warning GD616DC71: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807
util.cc(75,8): warning GA5933D32: ‘MEM[(struct timing_point *)&partial_time].ns’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Which, even though well-intentioned (and, technically, correct) are irrelevant
in our case and the compiler+linker diagnostics fail to understand the intention
and usage of the code.

Additionally, fix a few instances where type conversion is different for
different platforms/targets.

Desktop compilers mis-detect memory allocation size in our bundled property
code if fortified version of `strncpy` is used (even though the size
calculation is correct). Use `memcpy` instead.

LTO doesn't appear to be ready for use just yet. It gives warnings similar to
these:

    jni-wrappers.hh(143,41): warning GD616DC71: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807
    util.cc(75,8): warning GA5933D32: ‘MEM[(struct timing_point *)&partial_time].ns’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Which, even though well-intentioned (and, technically, correct) are irrelevant
in our case and the compiler+linker diagnostics fail to understand the intention
and usage of the code.

Additionally, fix a few instances where type conversion is different for
different platforms/targets.
@@ -192,8 +192,8 @@ AndroidSystem::_monodroid__system_property_get (const char *name, char *sp_value
// ../../../jni/android-system.cc(206,10): warning G20816D19: ‘char* strncpy(char*, const char*, size_t)’ specified bound 93 equals destination size [-Wstringop-truncation] [/home/grendel/vc/xamarin/xamarin-android-worktrees/code-quality-improvements/src/monodroid/monodroid.csproj]
// strncpy (sp_value, env_value, sp_value_len);
//
strncpy (sp_value, env_value, sp_value_len - 1);
sp_value[sp_value_len] = '\0';
strncpy (sp_value, env_value, sp_value_len - 2);
Copy link
Member

Choose a reason for hiding this comment

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

Given that we're using memcpy() elsewhere, should we just use memcpy() everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep using the string functions for string types elsewhere. The size calculations here appear to confuse the compiler which warns about code that is actually valid (on targets which have fortified string manipulation functions, e.g. in desktop builds), so using memcpy was the easiest way to shut the compiler up here.

@@ -124,7 +136,7 @@ namespace xamarin::android

std::lock_guard<std::mutex> lock (sequence_lock);
if (sequence->dynamic) {
memset (sequence, 0, sizeof(*sequence));
sequence->period.reset ();
Copy link
Member

Choose a reason for hiding this comment

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

This is odd...and I don't remember this coming up before... but why clear out period if it'll be invalidated on the next delete sequence line? Why is this done? (Same question applies to the original memset(), and I don't remember this being discussed previously with PR #3729.)

In principal, the delete sequence could (should?) "scribble" over *sequence so that if it's reused, it can be more readily detected in a debugger. We'd thus effectively have:

memset (sequence, 0, sizeof (*sequence);
memset (sequence, 0xbb, sizeof (*sequence);  // or some other "`free` was here!" constant

What's the memset()/period.reset() buy us?

Copy link
Contributor Author

@grendello grendello Nov 4, 2019

Choose a reason for hiding this comment

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

The reason clearing is done is that we don't control the IntPtr used to hold the sequence on the managed side. And clearing the sequence to 0 marks it as (sort of) "unused" should anything in the managed land reuse the pointer (which hopefully never happens). Doing the memset in our implementation of delete would unnecessarily slow things down.

Copy link
Member

Choose a reason for hiding this comment

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

Doing the memset in our implementation of delete would unnecessarily slow things down.

I wasn't advocating that our delete do that. I was just saying that many free()/operator delete() implementations do so.

@jonpryor jonpryor merged commit 59c24e1 into dotnet:master Nov 4, 2019
@grendello grendello deleted the cppify branch November 4, 2019 20:22
jonpryor pushed a commit that referenced this pull request Nov 8, 2019
Desktop compilers mis-detect memory allocation size in our bundled
property code if a fortified version of **strncpy**(3) is used (even
though the size calculation is correct).  Use **memcpy**(3) instead.

LTO doesn't appear to be ready for use just yet.  It gives warnings
similar to these:

	jni-wrappers.hh(143,41): warning GD616DC71: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807
	util.cc(75,8): warning GA5933D32: ‘MEM[(struct timing_point *)&partial_time].ns’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Which, even though well-intentioned (and, technically, correct) are
irrelevant in our case and the compiler+linker diagnostics fail to
understand the intention and usage of the code.

Additionally, fix a few instances where type conversion is different
for different platforms/targets.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants