[BUG] DexRewriter sample code

[Lanchon]

@JesusFreke,

It looks like this sample code:

smali/dexlib2/src/main/java/org/jf/dexlib2/rewriter/DexRewriter.java

Lines 58 to 60 in bf65e57

	* if (value.equals("Lorg/blah/MyBlah;")) {
	* return "Lorg/blah/YourBlah;";
	* }

does not map arrays of MyBlah.

is this true? or does dexlib2 dig down array types to get to the base type before invoking the type rewriter? thanks!

[Lanchon]

i've verified that this is indeed a bug.

i propose:

1. a) an abstract helper class be added called org.jf.dexlib2.rewriter.ElementalTypeRewriter (or NonArrayTypeRewriter or ArrayElementTypeRewriter) that extends TypeRewriter and adds an abstract rewriteElementalType(@Nonnull String elementalType) (or similar based on nonArrayType).

   b) alternatively, a non-abstract TypeRewriter.rewriteElementalType(...) method could be added. but this causes a small performance hit in code that extends the standard rewriter module but does not rewrite types. (this impact can be minimized -but not completely eliminated- by careful programming of the TypeRewriter.rewrite(...) method.)

   the sample code above should be fixed by using either of these solutions.

2. the definition of TypeRewriter.rewrite(...) be changed from rewrite(@Nonnull String value) to rewrite(@Nonnull String elementalOrArrayType) (or similar) to help implementers notice the caveat.

[Lanchon]

fix is here:
DexPatcher/dexpatcher-tool@c69cd60

that's probably the fastest solution that can be made, except -maybe- if want to use this incantation that i consider overkill:

if (rewrittenElementalType == elementalType || rewrittenElementalType.equals(elementalType)) ...

i hereby cede copyright of the linked commit in your favor, as if we had both written the same piece of code independently. you can claim copyright, license the code, and/or do whatever you want with it.

[JesusFreke]

Fixed by 78e92b0 (and slight fix in 409cf27)

I took a slightly different approach, adding functionality to "unwrap" array types in TypeRewriter.rewrite(), and passing both the non-array types and unwrapped array types to a new
rewriteUnwrappedType protected method.

[Lanchon]

this is exactly what i did in the previously linked commit (DexPatcher/dexpatcher-tool@c69cd60), only my code performs better.

your code:

• lastIndexOf unnecessarily scans the whole type.
• unnecessarily rebuilds the array type descriptor even if the underlying type hasn't changed (mine returns the original string instead; this is the expected hotspot case).
• unnecessarily allocates string arraySpecifiers.
• concatenates the resulting strings which can overflow the StringBuilder buffer and cause more copying.

go ahead and copy paste my code if you want.

[JesusFreke]

Thanks, I improved my implementation based on your comments (c1f2da0). One further improvement is the use of instance equality instead of value equality when comparing the input and output of rewriteUnwrappedType.

I actually didn't even see your second comment or your commit until after I had pushed mine and was adding my previous comment here. As you say, looks very similar, except I didn't give proper attention to the performance of this super critical piece of code ;)

[Lanchon]

One further improvement is the use of instance equality instead of value equality when comparing the input and output of rewriteUnwrappedType.

lol i mentioned this in my earlier comment (#738 (comment)), you should read more! :) but i consider it unnecessary. i don't think u'd be able to measure any difference because i know that the first thing String.equals() implementation does is check for identity and String is final so that check is trivially inlined by any compiler.

(regarding only checking identity and not content, i've got client code that can generate a new string with the same content for good reason. and again this is a hotspot case. in the rare cases when there's actually a type change, chances are the string comparison aborts early in the loop so avoiding StringBuilder in the hotspot case seems reasonable. but yes, this is all a matter of taste at this level.)

thanks!