Skip to content

Commit 8d1ab29

Browse files
simonrozsivalCopilot
simonrozsival
and
Copilot
committed
Fix managed JNI rewrite sequence handling
Associate legacy JNI member lookups with proven FindClass/Get*ID sequences, remove stale copied PDBs, and keep task outputs empty after rewrite failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent dac193e commit 8d1ab29

4 files changed

Lines changed: 552 additions & 56 deletions

File tree

‎src/Xamarin.Android.Build.Tasks/Tasks/RewriteJniNamesForR8.cs‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,14 @@ public class RewriteJniNamesForR8 : AndroidTask
4545

4646
public override bool RunTask ()
4747
{
48+
RewrittenFiles = [];
4849
if (DestinationDirectory.IsNullOrEmpty () && SourceFiles.Length != DestinationFiles.Length) {
4950
Log.LogCodedError ("XA4325", Properties.Resources.XA4325, Properties.Resources.XA4325_SourceDestinationCount);
5051
return !Log.HasLoggedErrors;
5152
}
5253

5354
R8Mapping mapping = R8Mapping.Load (MappingFile);
54-
var rewrittenFiles = new ITaskItem [SourceFiles.Length];
55+
var rewrittenFiles = new List<ITaskItem> (SourceFiles.Length);
5556

5657
for (int i = 0; i < SourceFiles.Length; i++) {
5758
string source = SourceFiles [i].ItemSpec;
@@ -64,16 +65,18 @@ public override bool RunTask ()
6465
ItemSpec = destination,
6566
};
6667
rewritten.SetMetadata ("OriginalItemSpec", source);
67-
rewrittenFiles [i] = rewritten;
68+
rewrittenFiles.Add (rewritten);
6869
} catch (JniRewriteException e) {
6970
Log.LogCodedError ("XA4325", Properties.Resources.XA4325,
7071
string.Format (Properties.Resources.XA4325_AssemblyFailure, source, e.Message));
7172
}
7273
}
7374

74-
RewrittenFiles = rewrittenFiles;
75-
if (!Log.HasLoggedErrors && !RewriteManifestFile.IsNullOrEmpty ()) {
76-
WriteRewriteManifest (RewriteManifestFile, mapping.AccessedEntries);
75+
if (!Log.HasLoggedErrors) {
76+
RewrittenFiles = rewrittenFiles.ToArray ();
77+
if (!RewriteManifestFile.IsNullOrEmpty ()) {
78+
WriteRewriteManifest (RewriteManifestFile, mapping.AccessedEntries);
79+
}
7780
}
7881
return !Log.HasLoggedErrors;
7982
}
@@ -114,9 +117,12 @@ void RewriteAssembly (string sourcePath, string destinationPath, R8Mapping mappi
114117
static void CopyAdjacentPdbUnchanged (string sourcePath, string destinationPath)
115118
{
116119
string pdbSource = Path.ChangeExtension (sourcePath, "pdb");
120+
string pdbDestination = Path.ChangeExtension (destinationPath, "pdb");
117121
if (File.Exists (pdbSource)) {
118-
string pdbDestination = Path.ChangeExtension (destinationPath, "pdb");
119122
Files.CopyIfChanged (pdbSource, pdbDestination);
123+
} else if (File.Exists (pdbDestination)) {
124+
Files.SetWriteable (pdbDestination);
125+
File.Delete (pdbDestination);
120126
}
121127
}
122128
}

‎src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/RewriteJniNamesForR8Tests.cs‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,46 @@ static byte [] BuildTrivialAssembly ()
3636
return stream.ToArray ();
3737
}
3838

39+
static byte [] BuildAssemblyWithMalformedLdstrOperand ()
40+
{
41+
var fixture = new JniFixtureBuilder ();
42+
UserStringHandle value = fixture.String ("malformed");
43+
int fieldStart = fixture.NextFieldRid;
44+
int methodStart = fixture.NextMethodRid;
45+
fixture.AddVoidMethod ("Malformed", fixture.EmitLoadStringBody (value));
46+
fixture.AddType ("Acme", "Malformed", fieldStart, methodStart);
47+
48+
byte [] image = fixture.Serialize ();
49+
uint token = (uint) MetadataTokens.GetToken (value);
50+
byte [] pattern = {
51+
(byte) ILOpCode.Ldstr,
52+
(byte) token,
53+
(byte) (token >> 8),
54+
(byte) (token >> 16),
55+
(byte) (token >> 24),
56+
(byte) ILOpCode.Pop,
57+
(byte) ILOpCode.Ret,
58+
};
59+
int match = -1;
60+
for (int i = 0; i <= image.Length - pattern.Length; i++) {
61+
bool matches = true;
62+
for (int j = 0; j < pattern.Length; j++) {
63+
if (image [i + j] != pattern [j]) {
64+
matches = false;
65+
break;
66+
}
67+
}
68+
if (!matches) {
69+
continue;
70+
}
71+
Assert.AreEqual (-1, match, "The fixture should contain exactly one matching ldstr sequence.");
72+
match = i;
73+
}
74+
Assert.AreNotEqual (-1, match, "The fixture's ldstr sequence was not found.");
75+
image [match + sizeof (uint)] = 0x71;
76+
return image;
77+
}
78+
3979
[Test]
4080
public void CopiesSourceToDestinationAndAdjacentPdbUnchanged ()
4181
{
@@ -83,6 +123,35 @@ public void CopiesSourceToDestinationAndAdjacentPdbUnchanged ()
83123
Assert.AreEqual ("Fixture", after.GetString (after.GetAssemblyDefinition ().Name));
84124
}
85125

126+
[Test]
127+
public void RemovesStaleDestinationPdbWhenSourceHasNoPdb ()
128+
{
129+
string path = Path.Combine (Root, "temp", TestName);
130+
Directory.CreateDirectory (path);
131+
132+
string sourceDll = Path.Combine (path, "source", "Test.dll");
133+
Directory.CreateDirectory (Path.GetDirectoryName (sourceDll));
134+
File.WriteAllBytes (sourceDll, BuildTrivialAssembly ());
135+
136+
string destinationDll = Path.Combine (path, "destination", "Test.dll");
137+
string destinationPdb = Path.ChangeExtension (destinationDll, "pdb");
138+
Directory.CreateDirectory (Path.GetDirectoryName (destinationDll));
139+
File.WriteAllBytes (destinationPdb, new byte [] { 1, 2, 3, 4 });
140+
141+
string mappingFile = Path.Combine (path, "mapping.txt");
142+
File.WriteAllText (mappingFile, "");
143+
var task = new RewriteJniNamesForR8 {
144+
BuildEngine = new MockBuildEngine (TestContext.Out),
145+
SourceFiles = new [] { new Microsoft.Build.Utilities.TaskItem (sourceDll) },
146+
DestinationFiles = new [] { new Microsoft.Build.Utilities.TaskItem (destinationDll) },
147+
MappingFile = mappingFile,
148+
};
149+
150+
Assert.IsTrue (task.Execute (), "Task should succeed.");
151+
FileAssert.Exists (destinationDll);
152+
FileAssert.DoesNotExist (destinationPdb, "A PDB from a previous copy must not survive when the source PDB is absent.");
153+
}
154+
86155
[Test]
87156
public void LeavesInPlaceAssemblyWithNoReplacementsUntouched ()
88157
{
@@ -132,6 +201,33 @@ public void FailsWithACodedErrorWhenSourceAndDestinationCountsDiffer ()
132201
StringAssert.Contains ("SourceFiles", errors [0].Message, "The error should name the mismatched item groups.");
133202
}
134203

204+
[Test]
205+
public void LeavesRewrittenFilesEmptyWhenAnAssemblyCannotBeRewritten ()
206+
{
207+
string path = Path.Combine (Root, "temp", TestName);
208+
Directory.CreateDirectory (path);
209+
string source = Path.Combine (path, "Malformed.dll");
210+
string destination = Path.Combine (path, "out", "Malformed.dll");
211+
File.WriteAllBytes (source, BuildAssemblyWithMalformedLdstrOperand ());
212+
213+
string mappingFile = Path.Combine (path, "mapping.txt");
214+
File.WriteAllText (mappingFile, "");
215+
var errors = new List<BuildErrorEventArgs> ();
216+
var task = new RewriteJniNamesForR8 {
217+
BuildEngine = new MockBuildEngine (TestContext.Out, errors),
218+
SourceFiles = new [] { new Microsoft.Build.Utilities.TaskItem (source) },
219+
DestinationFiles = new [] { new Microsoft.Build.Utilities.TaskItem (destination) },
220+
MappingFile = mappingFile,
221+
};
222+
223+
Assert.IsFalse (task.Execute (), "Task should fail for malformed IL.");
224+
Assert.AreEqual (1, errors.Count, "Exactly one error should have been logged.");
225+
Assert.AreEqual ("XA4325", errors [0].Code);
226+
StringAssert.Contains ("Malformed IL", errors [0].Message);
227+
CollectionAssert.IsEmpty (task.RewrittenFiles, "A failed invocation must not publish partial or null output items.");
228+
FileAssert.DoesNotExist (destination);
229+
}
230+
135231
[Test]
136232
public void HandlesMultipleFilesInOneInvocation ()
137233
{

‎src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/JniRemapping/JniAssemblyRewriterTests.cs‎

Lines changed: 154 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,24 @@ static BlobBuilder IntFieldSignature ()
118118
return signature;
119119
}
120120

121+
static BlobHandle AddLegacyJniMethodSignature (JniFixtureBuilder fixture, bool findClass)
122+
{
123+
var signature = new BlobBuilder ();
124+
new BlobEncoder (signature).MethodSignature ()
125+
.Parameters (findClass ? 1 : 3, out ReturnTypeEncoder returnType, out ParametersEncoder parameters);
126+
returnType.Type ().Int32 ();
127+
if (findClass) {
128+
parameters.AddParameter ().Type ().String ();
129+
} else {
130+
parameters.AddParameter ().Type ().Int32 ();
131+
parameters.AddParameter ().Type ().String ();
132+
parameters.AddParameter ().Type ().String ();
133+
}
134+
return fixture.Metadata.GetOrAddBlob (signature);
135+
}
136+
121137
[Test]
122-
public void RewritesBareMemberAndDescriptorForAReferencedJniClass ()
138+
public void DoesNotRewriteUnrelatedBareMemberAndDescriptorStrings ()
123139
{
124140
var fixture = new JniFixtureBuilder ();
125141
UserStringHandle className = fixture.String ("net/dot/android/ApplicationRegistration");
@@ -139,11 +155,147 @@ public void RewritesBareMemberAndDescriptorForAReferencedJniClass ()
139155
MetadataReader reader = peReader.GetMetadataReader ();
140156
CollectionAssert.AreEqual (new [] {
141157
"c4",
142-
"a",
158+
"Context",
143159
"Landroid/content/Context;",
144160
}, LoadedStrings (peReader, reader, method).ConvertAll (entry => entry.Value));
145161
}
146162

163+
[Test]
164+
public void RewritesLegacyJniLookupsForTwoClassesAndBothMethodHandleKinds ()
165+
{
166+
var fixture = new JniFixtureBuilder ();
167+
BlobHandle findClassSignature = AddLegacyJniMethodSignature (fixture, findClass: true);
168+
BlobHandle memberLookupSignature = AddLegacyJniMethodSignature (fixture, findClass: false);
169+
170+
int fieldStart = fixture.NextFieldRid;
171+
int methodStart = fixture.NextMethodRid;
172+
MethodDefinitionHandle findClassDefinition = fixture.Metadata.AddMethodDefinition (
173+
MethodAttributes.Public | MethodAttributes.Static | MethodAttributes.HideBySig,
174+
MethodImplAttributes.Runtime,
175+
fixture.Metadata.GetOrAddString ("FindClass"),
176+
findClassSignature,
177+
0,
178+
MetadataTokens.ParameterHandle (fixture.Metadata.GetRowCount (TableIndex.Param) + 1));
179+
MethodDefinitionHandle getStaticFieldDefinition = fixture.Metadata.AddMethodDefinition (
180+
MethodAttributes.Public | MethodAttributes.Static | MethodAttributes.HideBySig,
181+
MethodImplAttributes.Runtime,
182+
fixture.Metadata.GetOrAddString ("GetStaticFieldID"),
183+
memberLookupSignature,
184+
0,
185+
MetadataTokens.ParameterHandle (fixture.Metadata.GetRowCount (TableIndex.Param) + 1));
186+
fixture.AddType ("Android.Runtime", "JNIEnv", fieldStart, methodStart);
187+
188+
TypeReferenceHandle jniEnvironmentReference = fixture.Metadata.AddTypeReference (
189+
fixture.CoreLibraryReference,
190+
fixture.Metadata.GetOrAddString ("Android.Runtime"),
191+
fixture.Metadata.GetOrAddString ("JNIEnv"));
192+
MemberReferenceHandle findClassReference = fixture.Metadata.AddMemberReference (
193+
jniEnvironmentReference,
194+
fixture.Metadata.GetOrAddString ("FindClass"),
195+
findClassSignature);
196+
MemberReferenceHandle getMethodReference = fixture.Metadata.AddMemberReference (
197+
jniEnvironmentReference,
198+
fixture.Metadata.GetOrAddString ("GetMethodID"),
199+
memberLookupSignature);
200+
201+
UserStringHandle firstClass = fixture.String ("acme/one/First");
202+
UserStringHandle firstField = fixture.String ("count");
203+
UserStringHandle firstDescriptor = fixture.String ("I");
204+
UserStringHandle firstOtherField = fixture.String ("enabled");
205+
UserStringHandle firstOtherDescriptor = fixture.String ("Z");
206+
UserStringHandle secondClass = fixture.String ("acme/two/Second");
207+
UserStringHandle secondMethod = fixture.String ("run");
208+
UserStringHandle secondDescriptor = fixture.String ("()V");
209+
UserStringHandle ambiguousField = fixture.String ("state");
210+
211+
var localSignature = new BlobBuilder ();
212+
var localEncoder = new BlobEncoder (localSignature).LocalVariableSignature (2);
213+
localEncoder.AddVariable ().Type ().Int32 ();
214+
localEncoder.AddVariable ().Type ().Int32 ();
215+
StandaloneSignatureHandle locals = fixture.Metadata.AddStandaloneSignature (fixture.Metadata.GetOrAddBlob (localSignature));
216+
var controlFlow = new ControlFlowBuilder ();
217+
218+
fieldStart = fixture.NextFieldRid;
219+
methodStart = fixture.NextMethodRid;
220+
MethodDefinitionHandle method = fixture.AddVoidMethod ("LookUpBoth", fixture.EmitBody (encoder => {
221+
LabelHandle ambiguousLookup = encoder.DefineLabel ();
222+
223+
encoder.LoadString (firstClass);
224+
encoder.OpCode (ILOpCode.Call);
225+
encoder.Token (findClassDefinition);
226+
encoder.StoreLocal (0);
227+
encoder.LoadLocal (0);
228+
encoder.LoadString (firstField);
229+
encoder.LoadString (firstDescriptor);
230+
encoder.OpCode (ILOpCode.Call);
231+
encoder.Token (getStaticFieldDefinition);
232+
encoder.OpCode (ILOpCode.Pop);
233+
234+
encoder.LoadLocal (0);
235+
encoder.LoadString (firstOtherField);
236+
encoder.LoadString (firstOtherDescriptor);
237+
encoder.OpCode (ILOpCode.Call);
238+
encoder.Token (getStaticFieldDefinition);
239+
encoder.OpCode (ILOpCode.Pop);
240+
241+
encoder.LoadString (secondClass);
242+
encoder.OpCode (ILOpCode.Call);
243+
encoder.Token (findClassReference);
244+
encoder.StoreLocal (1);
245+
encoder.LoadLocal (1);
246+
encoder.LoadString (secondMethod);
247+
encoder.LoadString (secondDescriptor);
248+
encoder.OpCode (ILOpCode.Call);
249+
encoder.Token (getMethodReference);
250+
encoder.OpCode (ILOpCode.Pop);
251+
252+
encoder.LoadString (firstClass);
253+
encoder.OpCode (ILOpCode.Call);
254+
encoder.Token (findClassDefinition);
255+
encoder.StoreLocal (0);
256+
encoder.Branch (ILOpCode.Br_s, ambiguousLookup);
257+
encoder.LoadString (secondClass);
258+
encoder.OpCode (ILOpCode.Call);
259+
encoder.Token (findClassReference);
260+
encoder.StoreLocal (0);
261+
encoder.MarkLabel (ambiguousLookup);
262+
encoder.LoadLocal (0);
263+
encoder.LoadString (ambiguousField);
264+
encoder.LoadString (firstDescriptor);
265+
encoder.OpCode (ILOpCode.Call);
266+
encoder.Token (getStaticFieldDefinition);
267+
encoder.OpCode (ILOpCode.Pop);
268+
encoder.OpCode (ILOpCode.Ret);
269+
}, locals, controlFlow));
270+
fixture.AddType ("Acme", "LegacyLookups", fieldStart, methodStart);
271+
272+
JniRewriteResult result = Rewrite (fixture.Serialize (), Mapping (
273+
"acme.one.First -> a.b.F:\n" +
274+
" int count -> x\n" +
275+
" boolean enabled -> q\n" +
276+
" int state -> f\n" +
277+
"acme.two.Second -> a.b.S:\n" +
278+
" int state -> s\n" +
279+
" void run() -> y\n"));
280+
281+
using var peReader = new PEReader (ImmutableArray.Create (result.Image));
282+
MetadataReader reader = peReader.GetMetadataReader ();
283+
CollectionAssert.AreEqual (new [] {
284+
"a/b/F",
285+
"x",
286+
"I",
287+
"q",
288+
"Z",
289+
"a/b/S",
290+
"y",
291+
"()V",
292+
"a/b/F",
293+
"a/b/S",
294+
"state",
295+
"I",
296+
}, ValuesOf (LoadedStrings (peReader, reader, method)));
297+
}
298+
147299
[Test]
148300
public void RewritesAttributesAndLoadedStrings ()
149301
{

0 commit comments

Comments
 (0)