Fix --apimapping with unstructured parcelables
This fix consists of several related smaller fixes:
- --apimapping accepts positions arguments as inputs.
- --apimapping is a top-level command in HELP MSG.
- FOUND_PARCELABLE is handled as error consistently when -b is set.
- typo in aidl_parser_fuzzer.dict
Previously, FOUND_PARCELABLE error was returned from
load_and_validate_aidl and handled by callers. Treating it as error or
success was not consistent in various places.
Now, FOUND_PARCELABLE is returned only if -b is turned on and it is
always treated as an error by callers.
This fixes the crash that happens when --apimapping is called with
unstructured parcelables.
Bug: 196686928
Test: aidl_unittests
Test: m
Change-Id: I95cc9cc556825dc78f975d1f092b6420ec5399f8
diff --git a/aidl.cpp b/aidl.cpp
index ffddce1..e90fae1 100644
--- a/aidl.cpp
+++ b/aidl.cpp
@@ -487,12 +487,6 @@
// Validation phase
//////////////////////////////////////////////////////////////////////////
- // For legacy reasons, by default, compiling an unstructured parcelable (which contains no output)
- // is allowed. This must not be returned as an error until the very end of this procedure since
- // this may be considered a success, and we should first check that there are not other, more
- // serious failures.
- bool contains_unstructured_parcelable = false;
-
const auto& types = document->DefinedTypes();
const int num_defined_types = types.size();
for (const auto& defined_type : types) {
@@ -541,10 +535,8 @@
AIDL_ERROR(unstructured_parcelable)
<< "Refusing to generate code with unstructured parcelables. Declared parcelables "
"should be in their own file and/or cannot be used with --structured interfaces.";
- // Continue parsing for more errors
+ return AidlError::FOUND_PARCELABLE;
}
-
- contains_unstructured_parcelable = true;
}
if (defined_type->IsVintfStability()) {
@@ -675,11 +667,6 @@
*imported_files = import_paths;
}
- if (contains_unstructured_parcelable) {
- // Considered a success for the legacy case, so this must be returned last.
- return AidlError::FOUND_PARCELABLE;
- }
-
return AidlError::OK;
}
@@ -694,8 +681,7 @@
AidlError aidl_err = internals::load_and_validate_aidl(input_file, options, io_delegate,
&typenames, &imported_files);
- bool allowError = aidl_err == AidlError::FOUND_PARCELABLE && !options.FailOnParcelable();
- if (aidl_err != AidlError::OK && !allowError) {
+ if (aidl_err != AidlError::OK) {
return false;
}
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index cfa5d4a..672a25f 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -1143,6 +1143,15 @@
EXPECT_NE(nullptr, parse_result);
}
+TEST_F(AidlTest, ApiMappingAcceptsUnstructuredParcelables) {
+ io_delegate_.SetFileContents("p/Foo.aidl", "package p; parcelable Foo;");
+
+ Options options1 = Options::From("aidl --apimapping mapping.txt p/Foo.aidl");
+ CaptureStderr();
+ EXPECT_EQ(0, aidl_entry(options1, io_delegate_));
+ EXPECT_EQ("", GetCapturedStderr());
+}
+
TEST_F(AidlTest, FailOnParcelable) {
const string expected_foo_stderr =
"ERROR: p/IFoo.aidl:1.22-27: Refusing to generate code with unstructured parcelables. "
diff --git a/options.cpp b/options.cpp
index de695dc..f756e70 100644
--- a/options.cpp
+++ b/options.cpp
@@ -60,6 +60,15 @@
<< " Check whether NEW_DIR API dump is {compatible|equal} extension " << endl
<< " of the API dump OLD_DIR. Default: compatible" << endl
#endif
+ << endl
+ << myname_ << " --apimapping OUTPUT INPUT..." << endl
+ << " Generate a mapping of declared aidl method signatures to" << endl
+ << " the original line number. e.g.: " << endl
+ << " If line 39 of foo/bar/IFoo.aidl contains:"
+ << " void doFoo(int bar, String baz);" << endl
+ << " Then the result would be:" << endl
+ << " foo.bar.Baz|doFoo|int,String,|void" << endl
+ << " foo/bar/IFoo.aidl:39" << endl
<< endl;
// Legacy option formats
@@ -107,14 +116,6 @@
<< " tool, that part will not be traced." << endl
<< " --transaction_names" << endl
<< " Generate transaction names." << endl
- << " --apimapping" << endl
- << " Generates a mapping of declared aidl method signatures to" << endl
- << " the original line number. e.g.: " << endl
- << " If line 39 of foo/bar/IFoo.aidl contains:"
- << " void doFoo(int bar, String baz);" << endl
- << " Then the result would be:" << endl
- << " foo.bar.Baz|doFoo|int,String,|void" << endl
- << " foo/bar/IFoo.aidl:39" << endl
<< " -v VER, --version=VER" << endl
<< " Set the version of the interface and parcelable to VER." << endl
<< " VER must be an interger greater than 0." << endl
@@ -235,7 +236,7 @@
{0, 0, 0, 0},
};
const int c = getopt_long(argc, const_cast<char* const*>(argv.data()),
- "I:p:d:o:h:abtv:", long_options, nullptr);
+ "I:p:d:o:h:abtv:i:", long_options, nullptr);
if (c == -1) {
// no more options
break;
@@ -421,7 +422,8 @@
}
} else {
// the new arguments format
- if (task_ == Options::Task::COMPILE || task_ == Options::Task::DUMP_API) {
+ if (task_ == Options::Task::COMPILE || task_ == Options::Task::DUMP_API ||
+ task_ == Options::Task::DUMP_MAPPINGS) {
if (argc - optind < 1) {
error_message_ << "No input file." << endl;
return;
@@ -432,7 +434,7 @@
<< "got " << (argc - optind) << "." << endl;
return;
}
- if (task_ != Options::Task::CHECK_API && task_ != Options::Task::DUMP_MAPPINGS) {
+ if (task_ != Options::Task::CHECK_API) {
output_file_ = argv[optind++];
}
}
diff --git a/preprocess.cpp b/preprocess.cpp
index 643c871..2594ba6 100644
--- a/preprocess.cpp
+++ b/preprocess.cpp
@@ -126,7 +126,7 @@
AidlTypenames typenames;
auto result =
internals::load_and_validate_aidl(file, options, io_delegate, &typenames, nullptr);
- if (result == AidlError::OK || result == AidlError::FOUND_PARCELABLE) {
+ if (result == AidlError::OK) {
const auto& doc = typenames.MainDocument();
for (const auto& t : doc.DefinedTypes()) {
t->DispatchVisit(visitor);
diff --git a/tests/aidl_parser_fuzzer.dict b/tests/aidl_parser_fuzzer.dict
index 180b054..b8d3c0e 100644
--- a/tests/aidl_parser_fuzzer.dict
+++ b/tests/aidl_parser_fuzzer.dict
@@ -89,7 +89,7 @@
#arguments
" aidl "
-" --apimappings "
+" --apimapping "
" --checkapi=compatible "
" --checkapi=equal "
" --hash "