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

8272564: Incorrect attribution of method invocations of Object methods on interfaces #5165

Closed
wants to merge 3 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Aug 18, 2021

Consider these declarations:

    interface I {
        public String toString();
    }
    interface J extends I {}

There are two issues with the toString inherited from I into J:
-Trees.isAccessible(..., I.toString, J) will return false, because Resolve.isAccessible will return false, because Resolve.notOverriddenIn returns false, because the Object.toString method is found as an implementation of I.toString in the context of J. This is unfortunate, because Elements.getAllMembers(J) will return I.toString as a member, not Object.toString, so any API client listing all members and then validating them using Trees.isAccessible will filter toString out. The proposed solution is to avoid using the methods from java.lang.Object as implementations of interface methods in Resolve.notOverriddenIn. (Interfaces don't inherit from j.l.Object per my understanding of JLS.)
-as a slightly less problematic case, consider:

I i = null;
i.toString(); //AST and classfile contain call to I.toString()
J j = null;
j.toString(); //AST and classfile contain call to j.l.Object.toString()

I believe the second invocation should also preferably be a call to I.toString(), not a call to j.l.Object.toString(). The reason for this behavior is that in javac, interfaces have j.l.Object as a supertype, so method lookups over interfaces will look into j.l.Object before looking into the superinterfaces, and the concrete method will prevail over the super interface method found later. The proposal is, for interfaces, to only look into j.l.Object is a method is not found in the interface and its superinterfaces.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8272564: Incorrect attribution of method invocations of Object methods on interfaces

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5165/head:pull/5165
$ git checkout pull/5165

Update a local copy of the PR:
$ git checkout pull/5165
$ git pull https://git.openjdk.java.net/jdk pull/5165/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5165

View PR using the GUI difftool:
$ git pr show -t 5165

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5165.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 18, 2021

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Aug 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 18, 2021

@lahodaj The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the compiler label Aug 18, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 18, 2021

Webrevs

Loading

if (trees.isAccessible(s, name)) {
for (Element member : ct.getElements().getAllMembers(name)) {
if (!trees.isAccessible(s, member, (DeclaredType) name.asType())) {
trees.isAccessible(s, member, (DeclaredType) name.asType());
Copy link
Member

@JimLaskey JimLaskey Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this statement necessary?

Loading

Copy link
Contributor Author

@lahodaj lahodaj Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, that is a leftover from testing (so that I can easily put a breakpoint on the call when false).

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 18, 2021

@lahodaj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8272564: Incorrect attribution of method invocations of Object methods on interfaces

Reviewed-by: jlaskey, mcimadamore, vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 596 new commits pushed to the master branch:

  • 2e690ba: 8274322: Problems with oopDesc construction
  • a8edd1b: 8274527: Minimal VM build fails after JDK-8273459
  • 7326481: 8274393: Suppress more warnings on non-serializable non-transient instance fields in security libs
  • 8215b2e: 8274398: Suppress more warnings on non-serializable non-transient instance fields in management libs
  • 9573022: 8253197: vmTestbase/nsk/jvmti/StopThread/stopthrd007/TestDescription.java fails with "ERROR: DebuggeeSleepingThread: ThreadDeath lost"
  • f08180f: 8274501: c2i entry barriers read int as long on AArch64
  • c57ed22: 8274528: Add comment to explain an HKDF optimization in SSLSecretDerivation
  • 9180d9a: 8273216: JCMD does not work across container boundaries with Podman
  • 3e0d7c3: 8270290: NTLM authentication fails if HEAD request is used
  • bfd6163: 8274196: Crashes in VM_HeapDumper::work after JDK-8252842
  • ... and 586 more: https://git.openjdk.java.net/jdk/compare/a86ac0d1e3a6f02e587362c767abdf62b308d321...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Aug 18, 2021
@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Aug 18, 2021

I think that this one deserves a CSR, plus please consider also running the JCK tests just to double check

Loading

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Aug 18, 2021

/csr needed

Loading

@openjdk openjdk bot added the csr label Aug 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 18, 2021

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@lahodaj please create a CSR request for issue JDK-8272564. This pull request cannot be integrated until the CSR request is approved.

Loading

@openjdk openjdk bot removed the ready label Aug 18, 2021
@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Aug 19, 2021

question shouldn't field supertype_field in ClassType be assigned something different than j.l.Objet for interfaces with no super interface after your change?

Loading

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Aug 19, 2021

I think that this one deserves a CSR, plus please consider also running the JCK tests just to double check

Thanks, I've started to file the CSR here, please take a look:
https://bugs.openjdk.java.net/browse/JDK-8272564

question shouldn't field supertype_field in ClassType be assigned something different than j.l.Objet for interfaces with no super interface after your change?

Not having interfaces "subclass" j.l.Object would be an alternative variant of the fix (it would probably remove the need to these changes altogether). In my experiments, it seemed quite complex to do, however. I can try again, if desired.

Thanks for the comments!

Loading

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Aug 19, 2021

I think that this one deserves a CSR, plus please consider also running the JCK tests just to double check

Thanks, I've started to file the CSR here, please take a look:
https://bugs.openjdk.java.net/browse/JDK-8272564

question shouldn't field supertype_field in ClassType be assigned something different than j.l.Objet for interfaces with no super interface after your change?

Not having interfaces "subclass" j.l.Object would be an alternative variant of the fix (it would probably remove the need to these changes altogether). In my experiments, it seemed quite complex to do, however. I can try again, if desired.

Thanks for the comments!

yep I was also considering that it would be a big change, but seems like the right thing to do, I let it to your consideration

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

The implementation logic seems to be moving in the right direction.It is true that interface do not inherit members for Object - but the JLS is set up in a strange way:

 If an interface has no direct superinterface types, then the interface implicitly declares a public abstract member method m with signature s, return type r, and throws clause t corresponding to each public instance method m with signature s, return type r, and throws clause t declared in Object (§4.3.2), unless an abstract method with the same signature, same return type, and a compatible throws clause is explicitly declared by the interface. 

This seems to suggest that resolution should return I.toString even if I does NOT declare a toString method. That makes sense, after all, implementations of I will always have some toString(). This is not what your fallback Object lookup in findMethod does.

I also suggest running benchmarks, as the effect of these changes would be to generate invokeinterface where we used to have invokevirtual. VM should be smart enough to figure that one out, but better check.

Loading

@@ -1854,7 +1854,8 @@ private Symbol findMethod(Env<AttrContext> env,
List<Type>[] itypes = (List<Type>[])new List[] { List.<Type>nil(), List.<Type>nil() };

InterfaceLookupPhase iphase = InterfaceLookupPhase.ABSTRACT_OK;
for (TypeSymbol s : superclasses(intype)) {
boolean isInterface = site.tsym.isInterface();
for (TypeSymbol s : isInterface ? List.of(intype.tsym) : superclasses(intype)) {
Copy link
Contributor

@mcimadamore mcimadamore Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the code here is making it more difficult because we include the initial type into superclasses. Maybe it could be worth to have an initial round where we just look into the initial type - followed by an (optional) round where we look at superclasses, followed by a round where we look at superinterfaces, followed by a fallback (optional) round where we look at Object.

Loading

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Sep 9, 2021

Thanks for the comment @mcimadamore!

In 16ed87f, I changed the code to generate invokeinterface for methods from j.l.Object if invoked on an interface. I ran a JMH benchmark, and it does not seem to cause much issues for the sustained case:

Benchmark                        (p)   Mode  Cnt          Score          Error  Units
MyBenchmark.testNewDesugaring  param  thrpt   25  280021515.383 ± 11477634.990  ops/s
MyBenchmark.testOldDesugaring  param  thrpt   25  272013631.484 ±  9960654.146  ops/s

I also tried the benchmarks, but the results are more unclear there.

I tried to split the loop for the current type and the superclasses, but in the end it didn't look very nice, so I skipped that for now. I can push the code somewhere, if desired.

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

I think we can go ahead for now. Thanks.

Loading

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

looks good to me

Loading

@openjdk openjdk bot added ready and removed csr labels Oct 1, 2021
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Oct 5, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

Going to push as commit a5080ef.
Since your change was applied there have been 627 commits pushed to the master branch:

  • a914ee7: 8274632: Possible pointer overflow in PretouchTask chunk claiming
  • 8f7a37c: 8274434: move os::get_default_process_handle and os::dll_lookup to os_posix for POSIX platforms
  • 3953e07: 8271459: C2: Missing NegativeArraySizeException when creating StringBuilder with negative capacity
  • 53d7e95: 8274635: Use String.equals instead of String.compareTo in jdk.accessibility
  • e43f540: 8274651: Possible race in FontDesignMetrics.KeyReference.dispose
  • 2e542e3: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU
  • 7e757f6: 8274559: JFR: Typo in 'jfr help configure' text
  • 75d6688: 8274745: ProblemList TestSnippetTag.java
  • 9914e5c: 8274610: Add linux-aarch64 to bootcycle build profiles
  • 0ca094b: 8273244: Improve diagnostic output related to ErroneousTree
  • ... and 617 more: https://git.openjdk.java.net/jdk/compare/a86ac0d1e3a6f02e587362c767abdf62b308d321...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Oct 5, 2021
@openjdk openjdk bot added the integrated label Oct 5, 2021
@openjdk openjdk bot removed ready rfr labels Oct 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

@lahodaj Pushed as commit a5080ef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants