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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 4, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions build-tools/cmake/xa_macros.cmake
View file
Open in desktop
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
View file
Open in desktop
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);
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.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 ();
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 •
edited
Loading

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.

delete sequence;
return;
}
Expand Down