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

An apocalypse detector... #1140

Merged
merged 9 commits into from
Oct 20, 2016
Merged

An apocalypse detector... #1140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c0b5ec5
need support for returning ownership of strings; marshallers already …
eggrobin Oct 18, 2016
f8c5f65
add generated files for comparison
eggrobin Oct 18, 2016
c81f490
changes to the generated code
eggrobin Oct 18, 2016
4245f97
remove generated files
eggrobin Oct 18, 2016
3a4f142
changes to the generator
eggrobin Oct 18, 2016
5893b6a
a subtle bug
eggrobin Oct 18, 2016
93260c5
I can't get no apocalypse
eggrobin Oct 18, 2016
f461845
A log error for the error
eggrobin Oct 18, 2016
42939f5
lint
eggrobin Oct 20, 2016
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
13 changes: 9 additions & 4 deletions ksp_plugin/interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,12 +406,17 @@ void principia__GetVersion(
bool principia__HasEncounteredApocalypse(
Plugin* const plugin,
char const** const details) {
journal::Method<journal::HasEncounteredApocalypse> m({{plugin}, {details}});
journal::Method<journal::HasEncounteredApocalypse> m({plugin}, {details});
// Ownership will be transfered to the marshmallow.
std::string* const allocated_details = new std::string;
std::string details_string;
bool const has_encountered_apocalypse =
CHECK_NOTNULL(plugin)->HasEncounteredApocalypse(allocated_details);
*CHECK_NOTNULL(details) = allocated_details->c_str();
CHECK_NOTNULL(plugin)->HasEncounteredApocalypse(&details_string);
UniqueBytes allocated_details(details_string.size() + 1);
Copy link
Member

Choose a reason for hiding this comment

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

This seems very complicated. I don't think you need allocated_details at all: just copy details_string into *details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Someone has to allocate something releaseable; *details may well be null.
Note that the existing code with new std::string was broken, the pointer to the string was lost and thus leaked---and then the c_str() was deallocated, which is probably UB.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

std::memcpy(allocated_details.data.get(),
details_string.data(),
details_string.size() + 1);
*CHECK_NOTNULL(details) =
reinterpret_cast<char const*>(allocated_details.data.release());
return m.Return(has_encountered_apocalypse);
}

Expand Down
46 changes: 37 additions & 9 deletions ksp_plugin_adapter/ksp_plugin_adapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,18 @@ private enum PluginSource {

private String bad_installation_popup_;

// The first apocalyptic error message.
[KSPField(isPersistant = true)]
private String revelation_ = "";
// Whether we have encountered an apocalypse already.
[KSPField(isPersistant = true)]
private bool is_post_apocalyptic_ = false;
[KSPField(isPersistant = true)]
private int apocalypse_window_x_ = UnityEngine.Screen.width / 2;
[KSPField(isPersistant = true)]
private int apocalypse_window_y_ = UnityEngine.Screen.height / 3;
private UnityEngine.Rect apocalypse_window_rectangle_;

public event Action render_windows;

PrincipiaPluginAdapter() {
Expand Down Expand Up @@ -403,18 +415,14 @@ public override void OnAwake() {
public override void OnSave(ConfigNode node) {
base.OnSave(node);
if (PluginRunning()) {
IntPtr serialization = IntPtr.Zero;
String serialization;
IntPtr serializer = IntPtr.Zero;
for (;;) {
try {
serialization = plugin_.SerializePlugin(ref serializer);
if (serialization == IntPtr.Zero) {
break;
}
node.AddValue(principia_key, Marshal.PtrToStringAnsi(serialization));
} finally {
Interface.DeleteString(ref serialization);
serialization = plugin_.SerializePlugin(ref serializer);
if (serialization == null) {
break;
}
node.AddValue(principia_key, serialization);
}
}
}
Expand Down Expand Up @@ -484,6 +492,25 @@ private void OnGUI() {
bad_installation_popup_ = null;
return;
}

if (is_post_apocalyptic_) {
UnityEngine.GUI.skin = null;
apocalypse_window_rectangle_.xMin = apocalypse_window_x_;
apocalypse_window_rectangle_.yMin = apocalypse_window_y_;
apocalypse_window_rectangle_ = UnityEngine.GUILayout.Window(
id : this.GetHashCode(),
screenRect : apocalypse_window_rectangle_,
func : (int id) => {
UnityEngine.GUILayout.BeginVertical();
UnityEngine.GUILayout.TextArea(revelation_);
UnityEngine.GUILayout.EndVertical();
},
text : "Principia",
options : UnityEngine.GUILayout.MinWidth(500));
apocalypse_window_x_ = (int)apocalypse_window_rectangle_.xMin;
apocalypse_window_y_ = (int)apocalypse_window_rectangle_.yMin;
}

if (KSP.UI.Screens.ApplicationLauncher.Ready && toolbar_button_ == null) {
UnityEngine.Texture toolbar_button_texture;
LoadTextureOrDie(out toolbar_button_texture, "toolbar_button.png");
Expand Down Expand Up @@ -749,6 +776,7 @@ private void FixedUpdate() {
FloatingOrigin.SetOffset(displacement_offset);
krakensbane_.FrameVel += velocity_offset;
}
is_post_apocalyptic_ |= plugin_.HasEncounteredApocalypse(out revelation_);
}
}

Expand Down
16 changes: 16 additions & 0 deletions ksp_plugin_test/interface_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ using quantities::si::Second;
using quantities::si::Tonne;
using testing_utilities::AlmostEquals;
using ::testing::AllOf;
using ::testing::DoAll;
using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::Property;
Expand Down Expand Up @@ -803,6 +804,21 @@ TEST_F(InterfaceTest, SerializePlugin) {
EXPECT_THAT(serialization, IsNull());
}


TEST_F(InterfaceTest, Apocalypse) {
char const* details;
EXPECT_CALL(*plugin_, HasEncounteredApocalypse(_)).WillOnce(Return(false));
EXPECT_FALSE(principia__HasEncounteredApocalypse(plugin_.get(), &details));

constexpr char silly_string[] = "silly";
EXPECT_CALL(*plugin_, HasEncounteredApocalypse(_))
.WillOnce(DoAll(SetArgPointee<0>(silly_string), Return(true)));
EXPECT_TRUE(principia__HasEncounteredApocalypse(plugin_.get(), &details));
EXPECT_STREQ(silly_string, details);
principia__DeleteString(&details);
EXPECT_THAT(details, IsNull());
}

TEST_F(InterfaceTest, DeserializePlugin) {
PushDeserializer* deserializer = nullptr;
Plugin const* plugin = nullptr;
Expand Down
4 changes: 4 additions & 0 deletions ksp_plugin_test/mock_plugin.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@

#pragma once

#include <string>
#include <vector>

#include "base/not_null.hpp"
Expand Down Expand Up @@ -34,6 +35,9 @@ class MockPlugin : public Plugin {
MOCK_METHOD0(EndInitialization,
void());

MOCK_CONST_METHOD1(HasEncounteredApocalypse,
bool(std::string* const details));

MOCK_CONST_METHOD2(UpdateCelestialHierarchy,
void(Index const celestial_index,
Index const parent_index));
Expand Down
1 change: 1 addition & 0 deletions physics/ephemeris_body.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,7 @@ void Ephemeris<Frame>::AppendMassiveBodiesState(
Status(status.error(),
"Error extending trajectory for " + bodies_[i]->name() + ". " +
status.message());
LOG(ERROR) << "New Apocalypse: " << last_severe_integration_status_;
}

++index;
Expand Down
3 changes: 2 additions & 1 deletion serialization/journal.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,8 @@ message HasEncounteredApocalypse {
required fixed64 plugin = 1 [(pointer_to) = "Plugin", (is_subject) = true];
}
message Out {
required string details = 1 [(is_produced) = true];
required fixed64 details = 1 [(pointer_to) = "char const",
(is_produced) = true];
}
message Return {
required bool result = 1;
Expand Down
50 changes: 35 additions & 15 deletions tools/journal_proto_processor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,8 @@ void JournalProtoProcessor::ProcessOptionalNonStringField(
// instead.
field_cs_type_[descriptor] = cs_boxed_type;
field_cs_marshal_[descriptor] =
"[MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(OptionalMarshaler<" + cs_unboxed_type + ">))]";
"MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(OptionalMarshaler<" + cs_unboxed_type + ">))";
field_cxx_type_[descriptor] = cxx_type + " const*";

field_cxx_arguments_fn_[descriptor] =
Expand Down Expand Up @@ -374,6 +374,17 @@ void JournalProtoProcessor::ProcessRequiredFixed64Field(
};
}

// Special handlings for produced C-style strings: these are seen from the C#
// as strings, and marshalled with immediate destruction.
if (pointer_to == "char const" &&
(options.HasExtension(journal::serialization::is_produced) ||
options.HasExtension(journal::serialization::is_produced_if))) {
field_cs_type_[descriptor] = "String";
field_cs_marshal_[descriptor] =
"MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(OutOwnedUTF8Marshaler))";
}

field_cxx_deserializer_fn_[descriptor] =
[pointer_to](std::string const& expr) {
return "DeserializePointer<" + pointer_to + "*>(*pointer_map, " + expr +
Expand Down Expand Up @@ -440,10 +451,10 @@ void JournalProtoProcessor::ProcessRequiredUint32Field(
void JournalProtoProcessor::ProcessSingleStringField(
FieldDescriptor const* descriptor) {
field_cs_marshal_[descriptor] =
Contains(out_, descriptor) ? "[MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(OutUTF8Marshaler))]"
: "[MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(InUTF8Marshaler))]";
Contains(out_, descriptor) ? "MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(OutUTF8Marshaler))"
: "MarshalAs(UnmanagedType.CustomMarshaler, "
"MarshalTypeRef = typeof(InUTF8Marshaler))";
field_cs_type_[descriptor] = "String";
field_cxx_type_[descriptor] = "char const*";
FieldOptions const& options = descriptor->options();
Expand Down Expand Up @@ -719,7 +730,9 @@ void JournalProtoProcessor::ProcessInOut(

if (must_generate_code) {
cs_interface_parameters_[descriptor].push_back(
" " + Join({field_cs_marshal_[field_descriptor],
" " + Join({field_cs_marshal_[field_descriptor].empty()
? ""
: "[" + field_cs_marshal_[field_descriptor] + "]",
field_cs_mode_fn_[field_descriptor](
field_cs_type_[field_descriptor])},
/*joiner=*/" ") +
Expand Down Expand Up @@ -770,9 +783,11 @@ void JournalProtoProcessor::ProcessReturn(Descriptor const* descriptor) {
field_cxx_deserializer_fn_[field_descriptor](cxx_field_getter) +
" == result);\n";
}
cs_interface_return_type_[descriptor] =
Join({field_cs_marshal_[field_descriptor],
field_cs_type_[field_descriptor]}, /*joiner=*/" ");
cs_interface_return_marshal_[descriptor] =
field_cs_marshal_[field_descriptor].empty()
? ""
: "[return : " + field_cs_marshal_[field_descriptor] + "]";
cs_interface_return_type_[descriptor] = field_cs_type_[field_descriptor];
cxx_interface_return_type_[descriptor] = field_cxx_type_[field_descriptor];
cxx_nested_type_declaration_[descriptor] =
" using Return = " + field_cxx_type_[field_descriptor] + ";\n";
Expand Down Expand Up @@ -884,6 +899,7 @@ void JournalProtoProcessor::ProcessMethodExtension(
std::vector<std::string> cs_interface_parameters;
std::vector<std::string> cxx_interface_parameters;
std::vector<std::string> cxx_run_arguments;
std::string cs_interface_return_marshal = "";
std::string cs_interface_return_type = "void";
std::string cxx_interface_return_type = "void";
std::string cxx_run_prolog;
Expand Down Expand Up @@ -934,6 +950,8 @@ void JournalProtoProcessor::ProcessMethodExtension(
"not_null<Message*> const message) {\n" +
cxx_fill_body_[nested_descriptor] +
"}\n\n";
cs_interface_return_marshal =
cs_interface_return_marshal_[nested_descriptor];
cs_interface_return_type = cs_interface_return_type_[nested_descriptor];
cxx_interface_return_type = cxx_interface_return_type_[nested_descriptor];
}
Expand Down Expand Up @@ -986,11 +1004,13 @@ void JournalProtoProcessor::ProcessMethodExtension(
Join(cxx_run_arguments, /*joiner=*/", ") + ");\n";
cxx_functions_implementation_[descriptor] += cxx_run_epilog + "}\n\n";

cs_interface_method_declaration_[descriptor] =
" [DllImport(dllName : dll_path,\n"
" EntryPoint = \"principia__" + name + "\",\n"
" CallingConvention = CallingConvention.Cdecl)]\n"
" internal static extern " + cs_interface_return_type + " " + name + "(";
cs_interface_method_declaration_[descriptor] = Join(
{" [DllImport(dllName : dll_path,\n"
" EntryPoint = \"principia__" + name + "\",\n"
" CallingConvention = CallingConvention.Cdecl)]",
cs_interface_return_marshal,
"internal static extern " + cs_interface_return_type + " " + name + "("},
"\n ");
if (!cs_interface_parameters.empty()) {
cs_interface_method_declaration_[descriptor] +=
"\n " + Join(cs_interface_parameters, /*joiner=*/",\n "); // NOLINT
Expand Down
4 changes: 4 additions & 0 deletions tools/journal_proto_processor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ class JournalProtoProcessor {
std::map<Descriptor const*, std::string> cs_interface_return_type_;
std::map<Descriptor const*, std::string> cxx_interface_return_type_;

// The C# attribute for marshalling the return value of an interface method.
// The key is a descriptor for a Return message.
std::map<Descriptor const*, std::string> cs_interface_return_marshal_;

// The C#/C++ definition of a type corresponding to an interchange message.
// The key is a descriptor for an interchange message.
std::map<Descriptor const*, std::string> cs_interface_type_declaration_;
Expand Down