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.

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

ijar should produce valid class files with code attributes #13546

Open
szeiger opened this issue May 31, 2021 · 8 comments
Open

ijar should produce valid class files with code attributes #13546

szeiger opened this issue May 31, 2021 · 8 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-Java Issues for Java rules type: bug

Comments

@szeiger
Copy link

szeiger commented May 31, 2021

Description of the problem / feature request:

ijar strips out all code attributes, thus producing invalid classfiles that cannot be loaded into the JVM. Instead of removing code attributes it should replace them with a dummy implementation that throws an Exception.

Feature requests: what underlying problem are you trying to solve with this feature?

Our Scala tooling uses Zinc which may load dependent classes from the compiler classpath into the JVM when analyzing classes produced by javac in mixed Scala/Java compilation. This fails because the classfiles produced by ijar do not pass classfile validation:

java.lang.ClassFormatError: Absent Code attribute in method that is not native or abstract in class file org/apache/spark/ml/linalg/VectorUDT
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:757)
	at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
	at java.net.URLClassLoader.defineClass(URLClassLoader.java:468)
	at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at java.lang.Class.getDeclaredMethods0(Native Method)
	at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
	at java.lang.Class.privateGetPublicMethods(Class.java:2902)
	at java.lang.Class.getMethods(Class.java:1615)
	at sbt.internal.inc.ClassToAPI$.toDefinitions0(ClassToAPI.scala:164)
	at sbt.internal.inc.ClassToAPI$.$anonfun$toDefinitions$1(ClassToAPI.scala:121)
	at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:82)
	at sbt.internal.inc.ClassToAPI$.toDefinitions(ClassToAPI.scala:121)
	at sbt.internal.inc.ClassToAPI$.$anonfun$process$1(ClassToAPI.scala:28)
	at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:59)
	at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:52)
	at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:48)
	at sbt.internal.inc.ClassToAPI$.process(ClassToAPI.scala:28)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.readAPI$1(AnalyzingJavaCompiler.scala:143)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.$anonfun$compile$20(AnalyzingJavaCompiler.scala:161)
	at sbt.internal.inc.classfile.Analyze$.readInheritanceDependencies$1(Analyze.scala:148)
	at sbt.internal.inc.classfile.Analyze$.$anonfun$apply$14(Analyze.scala:154)
	at sbt.internal.inc.classfile.Analyze$.$anonfun$apply$14$adapted(Analyze.scala:81)
	at scala.collection.TraversableLike$WithFilter.$anonfun$foreach$1(TraversableLike.scala:789)
	at scala.collection.mutable.HashMap.$anonfun$foreach$1(HashMap.scala:138)
	at scala.collection.mutable.HashTable.foreachEntry(HashTable.scala:236)
	at scala.collection.mutable.HashTable.foreachEntry$(HashTable.scala:229)
	at scala.collection.mutable.HashMap.foreachEntry(HashMap.scala:40)
	at scala.collection.mutable.HashMap.foreach(HashMap.scala:138)
	at scala.collection.TraversableLike$WithFilter.foreach(TraversableLike.scala:788)
	at sbt.internal.inc.classfile.Analyze$.apply(Analyze.scala:81)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.$anonfun$compile$19(AnalyzingJavaCompiler.scala:161)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.$anonfun$compile$19$adapted(AnalyzingJavaCompiler.scala:159)
	at scala.collection.TraversableLike$WithFilter.$anonfun$foreach$1(TraversableLike.scala:789)
	at scala.collection.immutable.List.foreach(List.scala:389)
	at scala.collection.TraversableLike$WithFilter.foreach(TraversableLike.scala:788)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.$anonfun$compile$17(AnalyzingJavaCompiler.scala:159)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.timed(AnalyzingJavaCompiler.scala:196)
	at sbt.internal.inc.javac.AnalyzingJavaCompiler.compile(AnalyzingJavaCompiler.scala:159)
	at sbt.internal.inc.MixedAnalyzingCompiler.$anonfun$compile$4(MixedAnalyzingCompiler.scala:105)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
	at sbt.internal.inc.MixedAnalyzingCompiler.timed(MixedAnalyzingCompiler.scala:133)
	at sbt.internal.inc.MixedAnalyzingCompiler.compileJava$1(MixedAnalyzingCompiler.scala:90)
	at sbt.internal.inc.MixedAnalyzingCompiler.compile(MixedAnalyzingCompiler.scala:116)
	at sbt.internal.inc.IncrementalCompilerImpl.$anonfun$compileInternal$1(IncrementalCompilerImpl.scala:307)
	at sbt.internal.inc.IncrementalCompilerImpl.$anonfun$compileInternal$1$adapted(IncrementalCompilerImpl.scala:307)
	at sbt.internal.inc.Incremental$.doCompile(Incremental.scala:106)
	at sbt.internal.inc.Incremental$.$anonfun$compile$4(Incremental.scala:87)
	at sbt.internal.inc.IncrementalCommon.recompileClasses(IncrementalCommon.scala:116)
	at sbt.internal.inc.IncrementalCommon.cycle(IncrementalCommon.scala:63)
	at sbt.internal.inc.Incremental$.$anonfun$compile$3(Incremental.scala:89)
	at sbt.internal.inc.Incremental$.manageClassfiles(Incremental.scala:134)
	at sbt.internal.inc.Incremental$.compile(Incremental.scala:80)
	at sbt.internal.inc.IncrementalCompile$.apply(Compile.scala:67)
	at sbt.internal.inc.IncrementalCompilerImpl.compileInternal(IncrementalCompilerImpl.scala:311)
	at sbt.internal.inc.IncrementalCompilerImpl.$anonfun$compileIncrementally$1(IncrementalCompilerImpl.scala:269)
	at sbt.internal.inc.IncrementalCompilerImpl.handleCompilationError(IncrementalCompilerImpl.scala:159)
	at sbt.internal.inc.IncrementalCompilerImpl.compileIncrementally(IncrementalCompilerImpl.scala:238)
	at sbt.internal.inc.IncrementalCompilerImpl.compile(IncrementalCompilerImpl.scala:69)
...

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Sorry, I don't have a reproduction that I can share. This is based on our internal Scala rules, currently running on our Fork of Bazel 3.2.0.

@cushon
Copy link
Contributor

cushon commented Jun 3, 2021

I think this is a duplicate of #632

@szeiger
Copy link
Author

szeiger commented Jun 4, 2021

#632 is about preserving the code attributes for use in macros. This has nothing to do with macros and ijar should not preserve code in this case but generate stubs.

@sventiffe sventiffe added team-Rules-Java Issues for Java rules untriaged labels Jun 18, 2021
@comius comius added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Jul 21, 2021
@comius
Copy link
Contributor

comius commented Jul 21, 2021

I don't think it's a good idea to load ijar generated classes. Additionally such change would probably cause a regression - longer ijar class file.

@szeiger
Copy link
Author

szeiger commented Aug 2, 2021

I don't see how this would cause a regression. Yes, the ijars would get bigger, but they would still be deterministic.

@cushon
Copy link
Contributor

cushon commented Nov 24, 2021

Additionally such change would probably cause a regression - longer ijar class file.

I don't see how this would cause a regression. Yes, the ijars would get bigger

The motivation behind ijar is to improve build performance, so having ijar do additional work and produce slightly larger outputs would have some small effect there, the idea is that it's a potential performance regression.

Most compilers don't need to do regular classloading on the compile-time jars, or check that they have code attributes that verify.

Yannic added a commit to EngFlow/bazel that referenced this issue May 1, 2022
JVMS 4.7.3: "If the method is either native or abstract, its method_info
structure must not have a Code attribute. Otherwise, its method_info structure
must have exactly one Code attribute."

As hack around this, we instead emit a (dummy) Code attribute that does not
depend on the actual implementation of the method. This should fix compatibility
with tools like https://spotbugs.github.io which seem to care about the Code
attribute of dependencies and currently fail with ijar enabled.

Since there are some performance concerns about emitting a code attribute at
all, we start with only reading the Code attribute in this PR and add the code
to emit the dummy attributes in a follow-up to make benchmarking easier.

Progress on bazelbuild#13546
@cushon
Copy link
Contributor

cushon commented May 1, 2022

There's a related work-around for this in desugar:

/**
* Class loader that can "load" classes from header Jars. This class loader stubs in missing code
* attributes on the fly to make {@link ClassLoader#defineClass} happy. Classes loaded are unusable
* other than to resolve method references, so this class loader should only be used to process or
* inspect classes, not to execute their code. Also note that the resulting classes may be missing
* private members, which header Jars may omit.
*
* @see java.net.URLClassLoader
*/
public class HeaderClassLoader extends ClassLoader {

@cushon
Copy link
Contributor

cushon commented May 2, 2022

Thinking about this a little more, I wonder what the simplest code attributes we can get away with that would still verify are.

For regular non-void methods, throwing an exception should be sufficient.

For constructors, I think we need to invoke super-constructors, which isn't trivial. Now we're looking at examining the existing code to minimize it, or understanding the class hierarchy.

I think we don't need to worry about final fields if the constructor never completes normally.

Copy link

github-actions bot commented Feb 7, 2024

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-Java Issues for Java rules type: bug
Projects
None yet
Development

No branches or pull requests

5 participants