Skip to content

Commit

Permalink
[C++ification] Take care of a few warnings and don't use LTO (#3873)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
grendello authored and jonpryor committed Nov 8, 2019
1 parent efcff8b commit cdb8f8d
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 17 deletions.
8 changes: 0 additions & 8 deletions build-tools/cmake/xa_macros.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,6 @@ macro(xa_common_prepare)
fPIC
)

# Using flto seems to breaks LLDB debugging as debug symbols are not properly included
# thus disable on desktop builds where we care less about its benefits and would rather
# keep debuggability
if(NOT MINGW AND NOT WIN32 AND NOT APPLE)
# -flto leaves a lot of temporary files with mingw builds, turn the optimization off as we don't really need it there
set(XA_COMPILER_FLAGS ${XA_COMPILER_FLAGS} flto)
endif()

if(CMAKE_BUILD_TYPE STREQUAL Debug)
set(XA_COMPILER_FLAGS ${XA_COMPILER_FLAGS} ggdb3 fno-omit-frame-pointer O0)
else()
Expand Down
6 changes: 3 additions & 3 deletions src/monodroid/jni/android-system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ AndroidSystem::add_system_property (const char *name, const char *value)
return;

p->name = ((char*) p) + sizeof (struct BundledProperty);
strncpy (p->name, name, name_len);
memcpy (p->name, name, name_len);
p->name [name_len] = '\0';

if (value == nullptr) {
Expand Down Expand Up @@ -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);
sp_value[sp_value_len - 1] = '\0';

return static_cast<int>(strlen (sp_value));
}
Expand Down
3 changes: 1 addition & 2 deletions src/monodroid/jni/embedded-assemblies-zip.cc
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,7 @@ EmbeddedAssemblies::zip_read_field (uint8_t* buf, size_t buf_len, size_t index,
return false;
}

u = static_cast<uint16_t> (buf [index + 1] << 8) |
static_cast<uint16_t> (buf [index]);
u = static_cast<uint16_t>((buf [index + 1] << 8) | buf [index]);

return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/monodroid/jni/monodroid-glue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@ MonodroidRuntime::init_android_runtime (MonoDomain *domain, JNIEnv *env, jclass
init.localRefsAreIndirect = LocalRefsAreIndirect (env, runtimeClass, init.androidSdkVersion);
init.isRunningOnDesktop = is_running_on_desktop ? 1 : 0;
init.brokenExceptionTransitions = application_config.broken_exception_transitions ? 1 : 0;
init.packageNamingPolicy = application_config.package_naming_policy;
init.packageNamingPolicy = static_cast<int>(application_config.package_naming_policy);

// GC threshold is 90% of the max GREF count
init.grefGcThreshold = static_cast<int>(androidSystem.get_gref_gc_threshold ());
Expand Down
18 changes: 15 additions & 3 deletions src/monodroid/jni/timing.hh
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@ namespace xamarin::android
{
struct timing_point
{
time_t sec;
uint64_t ns;
time_t sec = 0;
uint64_t ns = 0;

void mark ();

void reset ()
{
sec = 0;
ns = 0;
}
};

struct timing_period
Expand All @@ -37,6 +43,12 @@ namespace xamarin::android
{
end.mark ();
}

void reset ()
{
start.reset ();
end.reset ();
}
};

struct timing_diff
Expand Down Expand Up @@ -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 ();
delete sequence;
return;
}
Expand Down

0 comments on commit cdb8f8d

Please sign in to comment.