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

8273408: java.lang.AssertionError: typeSig ERROR on generated class property of record #5511

Closed
wants to merge 6 commits into from

Conversation

@lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Sep 14, 2021

Hi all,

After processing annotation, if new source files are generated, the compiler should re-parse and re-enter the source files so that the type and symbol can keep right. The compiler always holds this convention in the past. But when the feature record is added, the convention is broken. In this bug, the compiler doesn't catch or enter the new/good type for record component after processing annotation. So some issues may occur at the later phases.

This patch fixes the type of the record component so that the phases Gen and ClassWriter can get the right type. And a test case is added.

Thanks for taking the time to review.

Best Regards,
-- Guoxiong


Progress

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

Issue

  • JDK-8273408: java.lang.AssertionError: typeSig ERROR on generated class property of record

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5511

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 14, 2021

👋 Welcome back gli! 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 Sep 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

@lgxbslgx 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 Sep 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2021

Webrevs

Loading

// Found a wrong record component: save it so that we can remove it later.
// If the class type of the record component is generated by annotation processor, it should
// use the new actual class type and symbol instead of the old dummy ErrorType.
// toRemove = rc;
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 15, 2021

Choose a reason for hiding this comment

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

there is dead code here, toRemove doesn't seem to be assigned ever, probably you uploaded an intermediate patch?

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Sep 15, 2021

Choose a reason for hiding this comment

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

The // toRemove = rc; should be toRemove = rc;. It is a statement instead of a comment. I revised it mitakenly in my second commit Polish. In the second commit Polish, I only want to fix the indentation so that I didn't test again after revising. Unfortunately, the IDE changed toRemove = rc; to a comment. Apology for the mistake.

Loading

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Sep 15, 2021

Hi all,

After processing annotation, if new source files are generated, the compiler should re-parse and re-enter the source files so that the type and symbol can keep right. The compiler always holds this convention in the past.

could you please add a link to where this is done in the compiler for other cases different from records? thanks

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Sep 15, 2021

could you please add a link to where this is done in the compiler for other cases different from records? thanks

The compiler would parse the newly generated source files and enter all the source files again. Many dummy types or symbols, such as ErrorType, would be replaced with the actually right ones.

But when the feature record is added, the convention is broken.

This description may be excessive. Actually, new features always obey the convention, so as record. But the method getRecordComponent in this bug is an example which breaks the convention.

In detail, please read the following source code, currently in the main line.

// Symbol.java
        public RecordComponent getRecordComponent(JCVariableDecl var, boolean addIfMissing, List<JCAnnotation> annotations) {
            for (RecordComponent rc : recordComponents) {
                /* it could be that a record erroneously declares two record components with the same name, in that
                 * case we need to use the position to disambiguate
                 */
                if (rc.name == var.name && var.pos == rc.pos) {
                    return rc;
                }
            }
            RecordComponent rc = null;
            if (addIfMissing) {
                recordComponents = recordComponents.append(rc = new RecordComponent(var.sym, annotations));
            }
            return rc;
        }

Before the first processing round, the var is not right at all, because var.sym.type is ErrorType. So the getRecordComponent would generate a not good RecordComponent by using the wrong var.sym whose type is ErrorType.

After the first processing round, the new source files are created and we can catch the right type from the newly generated source files. When the compiler invokes getRecordComponent to get the RecordComponent, it only can get the wrong one(the type is ErrorType). At this time, the var.sym.type is the right ClassType which is created correctly earier. So we should use current argument var and the right var.sym.type instead of the old wrong one.

So you can see my patch, I judge whether the type is right. If it is right, which means the RecordComponent created earier is right, I reuse it. If the type is ErrorType, we shouldn't keep it and should create a new one by using the right var whose var.sym.type is not ErrorType.

                    if (rc.type.hasTag(TypeTag.ERROR) && !var.sym.type.hasTag(TypeTag.ERROR)) {
                        // Found a wrong record component: save it so that we can remove it later.
                        // If the class type of the record component is generated by annotation processor, it should
                        // use the new actual class type and symbol instead of the old dummy ErrorType.
                        toRemove = rc;
                    } else {
                        // Found a good record component: just return.
                        return rc;
                    }

You can put breakpoints at the method getRecordComponent to watch the var.sym.type and rc.type. You can observe that the var.sym.type is an ErrorType at first and is a right ClassType after first processing round.

Loading

for (RecordComponent rc : recordComponents) {
/* it could be that a record erroneously declares two record components with the same name, in that
* case we need to use the position to disambiguate
*/
if (rc.name == var.name && var.pos == rc.pos) {
return rc;
if (rc.type.hasTag(TypeTag.ERROR) && !var.sym.type.hasTag(TypeTag.ERROR)) {
// Found a wrong record component: save it so that we can remove it later.
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 15, 2021

Choose a reason for hiding this comment

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

suggestion for the comment:

Found a record component with an erroneous type, save it so that it can be removed later.
If the class type of the ...

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Sep 16, 2021

Choose a reason for hiding this comment

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

Fixed.

Loading

}
}
RecordComponent rc = null;
if (addIfMissing) {
if (toRemove != null) {
// Found a wrong record component: remove it and create a new one.
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Sep 15, 2021

Choose a reason for hiding this comment

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

suggestion:

Found a record component with an erroneous type, remove it and create a new one

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Sep 16, 2021

Choose a reason for hiding this comment

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

Fixed.

Loading

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Sep 15, 2021

I think that there are two test cases missing here. I think that you should investigate the interaction of your patch with something like:

record R(unknownType t) {}

and see what happens, if your code creates another record component when I think it shouldn't, etc. Actually you could try to override the type in the existing record component instead of creating a new one.

Another investigation you should do is how your code interacts with annotations (declaration and type anotations) on the record component. So in your test add annotations to the record component and see if you need to copy them etc. I think that if you override the type of the record component instead of creating a new one you should be fine with annotations, but not sure TBH

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Sep 16, 2021

@vicente-romero-oracle thanks for your comment.

I think that there are two test cases missing here.

I added some tests about unknown type and annotation. Please take a look.

Actually you could try to override the type in the existing record component instead of creating a new one.

I don't think that overriding the type in the existing record component can solve this issue. As we can see the constructor of the RecordComponent, the current fields RecordComponent.type and RecordComponent.isVarargs depend on the type of the argument VarSymbol field(field.type). So we need to revise RecordComponent.type and RecordComponent.isVarargs in the method getRecordComponent. If the RecordComponent adds new fields which depend on the type of the argument VarSymbol field(field.type) in the future, the developers need to revise the method getRecordComponent to override the new fields. It is uncomfortable and the code is hard to maintain. So I consider that creating a new one is a better way.

        public RecordComponent(VarSymbol field, List<JCAnnotation> annotations) {
            super(PUBLIC, field.name, field.type, field.owner);   // <---- here uses `field.type`
            this.originalAnnos = annotations;
            this.pos = field.pos;
            /* it is better to store the original information for this one, instead of relying
             * on the info in the type of the symbol. This is because on the presence of APs
             * the symbol will be blown out and we won't be able to know if the original
             * record component was declared varargs or not.
             */
            this.isVarargs = type.hasTag(TypeTag.ARRAY) && ((ArrayType)type).isVarargs();  // <---- here `type` is `field.type`, too
        }

Loading

Copy link
Contributor

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

looks good, thanks for the additional research

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

@lgxbslgx 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:

8273408: java.lang.AssertionError: typeSig ERROR on generated class property of record

Reviewed-by: 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 4 new commits pushed to the master branch:

  • 59b2478: 8273659: Replay compilation crashes with SIGSEGV since 8271911
  • 5e4d09c: 8273300: Check Mutex ranking during a safepoint
  • c86e24d: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Sep 16, 2021
@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Sep 17, 2021

@vicente-romero-oracle Thanks for your review.

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2021

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

  • 8c022e2: 8270434: JDI+UT: Unexpected event in JDI tests
  • b982904: 8271073: Improve testing with VM option VerifyArchivedFields
  • bc48a0a: 8273902: Memory leak in OopStorage due to bug in OopHandle::release()
  • 9c5441c: 8271569: Clean up the use of CDS constants and field offsets
  • 12fa707: 8261941: Use ClassLoader for unregistered classes during -Xshare:dump
  • 7e92abe: 8273710: Remove redundant stream() call before forEach in jdk.jdeps
  • 59b2478: 8273659: Replay compilation crashes with SIGSEGV since 8271911
  • 5e4d09c: 8273300: Check Mutex ranking during a safepoint
  • c86e24d: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Sep 17, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2021

@lgxbslgx Pushed as commit e07ab82.

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

Loading

@lgxbslgx lgxbslgx deleted the JDK-8273408 branch Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants