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

Fix tracepoint specifiers #11173

Merged
merged 1 commit into from
Nov 13, 2020
Merged

Conversation

JasonFengJ9
Copy link
Member

@JasonFengJ9 JasonFengJ9 commented Nov 12, 2020

Changed %llx/%llu to %zx/%zu for UDATA.

Signed-off-by: Jason Feng fengj@ca.ibm.com

@JasonFengJ9
Copy link
Member Author

@gacholio could you please review?

@gacholio
Copy link
Contributor

I don't think you need to obsolete tracepoints when changing the formats here. Fixing them in place should be fine - the old tracepoints either worked or they didn't, the new formats are presumably correct.

@keithc-ca Any comments?

@JasonFengJ9
Copy link
Member Author

As per the rules for adding tracepoint definitions to TDF files:
If a tracepoint's signature changes, OBSOLETE IT AND CREATE A NEW ONE. By signature we mean the types, order and total number of format specifiers in the Template. Changes to the Tracepoint Type, Overhead, Level and NoEnv parameters, and cosmetic changes to the text in the Template can be made without adding a new tracepoint.
Technically this PR modifies the type which falls into the signature change category, however I do agree that

Fixing them in place should be fine - the old tracepoints either worked or they didn't, the new formats are presumably correct.

Just for the record, the tracepoints in question went in ~2 month ago, so the impact was only in last release cycle.

@gacholio
Copy link
Contributor

The "signature" of the tracepoints are not changing - you're just correcting the specifiers. The only possible issue I see is having the updates make it into the translations. This would be a poor reason for duplicating the tracepoints (you could update the translations yourself if this is going to be an issue).

@pshipton
Copy link
Member

Yes, if they are already translated then the translations should be updated as well. Not sure if a future translation pass would pick up the change. Also it will be ~8 months before another translation pass occurs.

@JasonFengJ9
Copy link
Member Author

Yes, if they are already translated then the translations should be updated as well. Not sure if a future translation pass would pick up the change. Also it will be ~8 months before another translation pass occurs.

How do we know if the translations for the last release have been done?

@pshipton
Copy link
Member

How do we know if the translations for the last release have been done?

Ya, actually these are tracepoints, they don't get translated. Some confusion with NLS messages here.

@JasonFengJ9
Copy link
Member Author

Modified the specifier within current tracepoints, please have another look.

Changed %llx/%llu to %zx/%zu

Signed-off-by: Jason Feng <fengj@ca.ibm.com>
@JasonFengJ9 JasonFengJ9 changed the title Fix tracepoint types Fix tracepoint specifiers Nov 13, 2020
Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

I did a few spot checks. The changes look good to me.

@keithc-ca keithc-ca self-assigned this Nov 13, 2020
@keithc-ca
Copy link
Contributor

Jenkins test sanity win32 jdk8

@keithc-ca
Copy link
Contributor

Jenkins test sanity zlinux jdk11

Copy link
Contributor

@gacholio gacholio left a comment

Choose a reason for hiding this comment

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

Sorry for the translation confusion.

@gacholio gacholio merged commit c4778b5 into eclipse-openj9:master Nov 13, 2020
@JasonFengJ9 JasonFengJ9 deleted the fixtracepoints branch November 26, 2020 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants