Skip to content

Commit

Permalink
Handle enums with renamed $VALUES field
Browse files Browse the repository at this point in the history 
Summary:
If a `$VALUES` field is not immediately found, do a simple analysis of the `value()` method to try to find the renamed field.

Add test cases.

Reviewed By: NTillmann

Differential Revision: D48800080

fbshipit-source-id: 52e473517121ab8797fe27b289354b7a6c247dc0
  • Loading branch information
@agampe @facebook-github-bot
agampe authored and facebook-github-bot committed Aug 30, 2023
1 parent 64b74d4 commit 53603c3
Show file tree
Hide file tree
Showing 2 changed files with 252 additions and 2 deletions.
109 changes: 107 additions & 2 deletions opt/rearrange-enum-clinit/RearrangeEnumClinit.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "RearrangeEnumClinit.h"

#include <atomic>
#include <optional>

#include "Debug.h"
#include "DexUtil.h"
Expand All @@ -17,6 +18,7 @@
#include "PassManager.h"
#include "ScopedCFG.h"
#include "Show.h"
#include "TypeUtil.h"
#include "Walkers.h"

namespace rearrange_enum_clinit {
Expand Down Expand Up @@ -55,8 +57,7 @@ struct Rearranger {
}(cfg.entry_block())) {}

IRInstruction* find_values_sput() {
// TODO(T162335058): Needs to be generalized to look at the `values()`
// function.
// Optimistically look for the `$VALUES` field and accept it.
for (auto it = b->rbegin(); it != b->rend(); ++it) {
if (it->type == MFLOW_OPCODE &&
it->insn->opcode() == OPCODE_SPUT_OBJECT) {
Expand All @@ -67,6 +68,110 @@ struct Rearranger {
}
}
}

// Look for the `values()` function and analyze it.
auto* c = type_class(m->get_class());
redex_assert(c != nullptr);

auto* values_method = [&c]() -> DexMethod* {
for (auto* dm : c->get_dmethods()) {
if (dm->get_name()->str() == "values") {
auto* p = dm->get_proto();
if (p->get_args()->empty() && type::is_array(p->get_rtype())) {
return dm;
}
}
}
return nullptr;
}();

if (values_method == nullptr) {
return nullptr;
}

auto* field = analyze_values_method(values_method);
if (field == nullptr) {
return nullptr;
}

for (auto it = b->rbegin(); it != b->rend(); ++it) {
if (it->type == MFLOW_OPCODE &&
it->insn->opcode() == OPCODE_SPUT_OBJECT) {
if (field == it->insn->get_field()) {
return it->insn;
}
}
}

return nullptr;
}

static DexFieldRef* analyze_values_method(DexMethod* values_method) {
cfg::ScopedCFG cfg(values_method->get_code());

std::optional<IRInstruction*> ret_opt{};
for (auto& mie : cfg::InstructionIterable(*cfg)) {
if (mie.insn->opcode() == OPCODE_RETURN_OBJECT) {
if (ret_opt) {
return nullptr; // Single return only.
}
ret_opt = mie.insn;
}
}
redex_assert(ret_opt);

using namespace live_range;
MoveAwareChains mac(*cfg);
auto use_def = mac.get_use_def_chains();

auto get_singleton = [&use_def](IRInstruction* insn,
src_index_t idx) -> IRInstruction* {
auto it = use_def.find(Use{insn, idx});
if (it == use_def.end()) {
return nullptr;
}
if (it->second.size() != 1) {
return nullptr;
}
return *it->second.begin();
};

for (IRInstruction* insn = *ret_opt; insn != nullptr;) {
// Written this way to ensure safe coding, always make progress.
src_index_t use_idx;
switch (insn->opcode()) {
case OPCODE_RETURN_OBJECT:
case OPCODE_CHECK_CAST:
use_idx = 0;
break;

case OPCODE_SGET_OBJECT: {
auto* f = insn->get_field();
if (f->get_class() == values_method->get_class() &&
type::get_element_type_if_array(f->get_type()) == f->get_class()) {
return f;
}
return nullptr;
}

case OPCODE_INVOKE_VIRTUAL: {
auto* mref = insn->get_method();
// Only support `clone()`.
if (mref->get_name()->str() != "clone" ||
mref->get_proto()->get_rtype() != type::java_lang_Object()) {
return nullptr;
}
use_idx = 0;
} break;

// Unsupported opcodes.
default:
return nullptr;
}

insn = get_singleton(insn, use_idx);
}

return nullptr;
}

Expand Down
145 changes: 145 additions & 0 deletions test/unit/RearrangeEnumClinitTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
* LICENSE file in the root directory of this source tree.
*/

#include "gtest/gtest.h"
#include <atomic>
#include <memory>
#include <system_error>
#include <unordered_map>
#include <unordered_set>

Expand Down Expand Up @@ -50,6 +52,10 @@ class RearrangeEnumClinitTest : public RedexTest {
type::make_array_type(self))
->make_concrete(ACC_PUBLIC | ACC_STATIC | ACC_FINAL));

cc.add_field(DexField::make_field(self, DexString::make_string("OTHER"),
type::make_array_type(self))
->make_concrete(ACC_PUBLIC | ACC_STATIC | ACC_FINAL));

cc.add_method(DexMethod::make_method("LTest;.<init>:(Ljava/lang/String;I)V")
->make_concrete(ACC_PUBLIC | ACC_CONSTRUCTOR, false));

Expand All @@ -58,6 +64,10 @@ class RearrangeEnumClinitTest : public RedexTest {
->make_concrete(ACC_PUBLIC | ACC_STATIC | ACC_CONSTRUCTOR, false);
cc.add_method(clinit);

values = DexMethod::make_method("LTest;.values:()[LTest;")
->make_concrete(ACC_PUBLIC | ACC_STATIC, false);
cc.add_method(values);

cc.create();
}

Expand All @@ -72,6 +82,7 @@ class RearrangeEnumClinitTest : public RedexTest {
}

DexMethod* clinit;
DexMethod* values;
};

TEST_F(RearrangeEnumClinitTest, Sgets) {
Expand Down Expand Up @@ -215,4 +226,138 @@ TEST_F(RearrangeEnumClinitTest, Regs) {
EXPECT_EQ(normalize(dst), assembler::to_string(code.get()));
}

// Subclass to separate things a little.
class RearrangeEnumClinitFieldTest : public RearrangeEnumClinitTest {
protected:
testing::AssertionResult check_other(const std::string& values_src) {
auto values_code = assembler::ircode_from_string(values_src);
values->set_code(std::move(values_code));

auto src = R"(
(
(const v4 2)
(const v3 1)
(const v2 0)
(new-instance "LTest;")
(move-result-pseudo-object v0)
(const-string ALPHA)
(move-result-pseudo-object v1)
(invoke-direct (v0 v1 v2) "LTest;.<init>:(Ljava/lang/String;I)V")
(sput-object v0 "LTest;.ALPHA:LTest;")
(move-object v16 v0)
(new-instance "LTest;")
(move-result-pseudo-object v0)
(const-string BETA)
(move-result-pseudo-object v1)
(invoke-direct (v0 v1 v3) "LTest;.<init>:(Ljava/lang/String;I)V")
(sput-object v0 "LTest;.BETA:LTest;")
(move-object v17 v0)
(new-instance "LTest;")
(move-result-pseudo-object v0)
(const-string GAMMA)
(move-result-pseudo-object v1)
(invoke-direct (v0 v1 v4) "LTest;.<init>:(Ljava/lang/String;I)V")
(sput-object v0 "LTest;.GAMMA:LTest;")
(move-object v18 v0)
(const v0 3)
(new-array v0 "[LTest;")
(move-result-pseudo-object v0)
(move-object v1 v16)
(aput-object v1 v0 v2)
(move-object v1 v17)
(aput-object v1 v0 v3)
(move-object v1 v18)
(aput-object v1 v0 v4)
(sput-object v0 "LTest;.OTHER:[LTest;")
(return-void)
)
)";

auto code = assembler::ircode_from_string(src);
auto res = run(code.get());

if (res != MethodResult::kChanged) {
return testing::AssertionFailure() << "Optimization not applied";
}

auto dst = R"(
(
(const v19 3)
(new-array v19 "[LTest;")
(move-result-pseudo-object v20)
(const v4 2)
(const v3 1)
(const v2 0)
(new-instance "LTest;")
(move-result-pseudo-object v0)
(const-string ALPHA)
(move-result-pseudo-object v1)
(invoke-direct (v0 v1 v2) "LTest;.<init>:(Ljava/lang/String;I)V")
(const v21 0)
(aput-object v0 v20 v21)
(sput-object v0 "LTest;.ALPHA:LTest;")
(move-object v16 v0)
(new-instance "LTest;")
(move-result-pseudo-object v0)
(const-string BETA)
(move-result-pseudo-object v1)
(invoke-direct (v0 v1 v3) "LTest;.<init>:(Ljava/lang/String;I)V")
(const v21 1)
(aput-object v0 v20 v21)
(sput-object v0 "LTest;.BETA:LTest;")
(move-object v17 v0)
(new-instance "LTest;")
(move-result-pseudo-object v0)
(const-string GAMMA)
(move-result-pseudo-object v1)
(invoke-direct (v0 v1 v4) "LTest;.<init>:(Ljava/lang/String;I)V")
(const v21 2)
(aput-object v0 v20 v21)
(sput-object v0 "LTest;.GAMMA:LTest;")
(move-object v18 v0)
(const v0 3)
(move-object v1 v16)
(move-object v1 v17)
(move-object v1 v18)
(sput-object v20 "LTest;.OTHER:[LTest;")
(return-void)
)
)";

auto normalized = normalize(dst);
auto code_str = assembler::to_string(code.get());
if (normalized != code_str) {
return testing::AssertionFailure() << "Unexpected output:\nExpected:\n"
<< normalized << "\nActual:\n"
<< code_str;
}
return testing::AssertionSuccess();
}
};

TEST_F(RearrangeEnumClinitFieldTest, OtherDirect) {
auto values_src = R"(
(
(sget-object "LTest;.OTHER:[LTest;")
(move-result-pseudo-object v0)
(return-object v0)
)
)";
ASSERT_TRUE(check_other(values_src));
}

TEST_F(RearrangeEnumClinitFieldTest, OtherClone) {
auto values_src = R"(
(
(sget-object "LTest;.OTHER:[LTest;")
(move-result-pseudo-object v0)
(invoke-virtual (v0) "LTest;.clone:()Ljava/lang/Object;")
(move-result-pseudo-object v0)
(check-cast v0 "[LTest;")
(return-object v0)
)
)";
ASSERT_TRUE(check_other(values_src));
}

} // namespace rearrange_enum_clinit

0 comments on commit 53603c3

Please sign in to comment.