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.

Sign up for GitHub

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

Jump to bottom

Z: Prioritize buffer size over buffer pointer in atoe_vsnprintf to better match std vsnprintf #7158

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Z: Prioritize buffer size over buffer pointer in atoe_vsnprintf to better match std vsnprintf #7158

merged 1 commit into from
Nov 9, 2023

Conversation

ehsankianifar
Copy link
Contributor

@ehsankianifar ehsankianifar commented Oct 25, 2023 •
edited

Std vsnprintf accepts null buffer and returns an estimate Our implementation does not support that
Updated the atoe_vsnprintf implementation

Signed-off-by: ehsan kiani far ehsan.kianifar@gmail.com

@ehsankianifar
Copy link
Contributor Author

@r30shah @joransiu This is the minimal changes pr. there are other issues with our implementation but I decided to minimize changes just to fix the current issue.

ehsankianifar
ehsankianifar commented Oct 25, 2023
View reviewed changes
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
ehsankianifar
ehsankianifar commented Oct 26, 2023
View reviewed changes
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
@ehsankianifar ehsankianifar changed the title Z: Fix atoe_vsnprintf to support null buffer WIP: Z: Fix atoe_vsnprintf to support null buffer Oct 26, 2023
@ehsankianifar ehsankianifar changed the title WIP: Z: Fix atoe_vsnprintf to support null buffer Z: Fix atoe_vsnprintf to support null buffer Oct 26, 2023
@ehsankianifar ehsankianifar mentioned this pull request Oct 26, 2023
Open
@r30shah
Copy link
Contributor

r30shah commented Oct 26, 2023

@ehsankianifar Can you fix the line ending errors reported in https://dev.azure.com/eclipse-omr/omr/_build/results?buildId=8104&view=logs&j=011e1ec8-6569-5e69-4f06-baf193d1351e&t=b8cbf579-6a08-5e02-2788-e25b5bd52b1b?

@ehsankianifar ehsankianifar force-pushed the Z-FixVsnprintNullBug branch 3 times, most recently from 38790e3 to 5f48768 Compare October 27, 2023 15:59
@ehsankianifar ehsankianifar mentioned this pull request Oct 27, 2023
Merged
@ehsankianifar
Copy link
Contributor Author

There is a test that fails on openj9, therefore we need to update the test as well. here is the openj9 pr: eclipse-openj9/openj9#18362

r30shah
r30shah suggested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

I need to give this change another look to make sure that we are not making assumptions which are incorrect.

fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
joransiu
joransiu suggested changes Oct 31, 2023
View reviewed changes
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
@ehsankianifar ehsankianifar force-pushed the Z-FixVsnprintNullBug branch 3 times, most recently from 3cb6426 to 7c4eed3 Compare October 31, 2023 16:20
r30shah
r30shah suggested changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

@ehsankianifar Instead of reviewing the change, I left a comment to make this API adhere to SPEC. I think we should get away without allocating temp buffer.

util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
@ehsankianifar ehsankianifar force-pushed the Z-FixVsnprintNullBug branch 4 times, most recently from 9e442c5 to c5dacca Compare November 1, 2023 20:43
r30shah
r30shah suggested changes Nov 2, 2023
View reviewed changes
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
@r30shah
Copy link
Contributor

r30shah commented Nov 6, 2023

@ehsankianifar Can you rebase your branch and push it ?

@ehsankianifar
Copy link
Contributor Author

@ehsankianifar Can you rebase your branch and push it ?

Sure, I am am working on it.

keithc-ca
keithc-ca suggested changes Nov 6, 2023
View reviewed changes
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
@ehsankianifar ehsankianifar changed the title Z: Fix atoe_vsnprintf to support null buffer Z: Prioritize buffer size over buffer pointer in atoe_vsnprintf to better match std vsnprintf Nov 6, 2023
keithc-ca
keithc-ca suggested changes Nov 7, 2023
View reviewed changes
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
keithc-ca
keithc-ca suggested changes Nov 7, 2023
View reviewed changes
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
util/a2e/atoe_utils.c Show resolved Hide resolved
keithc-ca
keithc-ca reviewed Nov 7, 2023
View reviewed changes
util/a2e/atoe_utils.c Outdated Show resolved Hide resolved
keithc-ca
keithc-ca suggested changes Nov 8, 2023
View reviewed changes
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
@ehsankianifar ehsankianifar force-pushed the Z-FixVsnprintNullBug branch 2 times, most recently from c91c427 to a066081 Compare November 8, 2023 19:36
keithc-ca
keithc-ca suggested changes Nov 8, 2023
View reviewed changes
fvtest/porttest/omrstrTest.cpp Show resolved Hide resolved
fvtest/porttest/omrstrTest.cpp Outdated Show resolved Hide resolved
@ehsankianifar
Z: Fix atoe_vsnprintf to support null buffer
5454992 
Std vsnprintf accepts null buffer and returns an estimate
Our implementation does not support that
Updated the atoe_vsnprintf implementation

Signed-off-by: ehsan kiani far <ehsan.kianifar@gmail.com>
keithc-ca
keithc-ca approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@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.

Merging this needs to be coordinated with merging eclipse-openj9/openj9#18362, else z/OS builds will be broken.

@r30shah
Copy link
Contributor

r30shah commented Nov 8, 2023

Jenkins build zos,zlinux

@r30shah
Copy link
Contributor

r30shah commented Nov 8, 2023

Thanks a lot @keithc-ca for a thorough review.
@ehsankianifar To make sure, if this goes first, then OpenJ9 PR (eclipse-openj9/openj9#18362) should go right after before next OMR acceptance right ?

@keithc-ca
Copy link
Member

this goes first, then OpenJ9 PR (eclipse-openj9/openj9#18362) should go right after before next OMR acceptance right ?

Yes, that's what we need/want.

@r30shah
Copy link
Contributor

r30shah commented Nov 9, 2023

@hzongaro / @0xdaryl Can we ask for your help to merge this one? This one fixes the issue that arises with refactoring done in Idiom Recognition some time back (those change uncovered issue on z/OS).

@ehsankianifar
Copy link
Contributor Author

ehsankianifar commented Nov 9, 2023 •
edited

build pipelines with all sanity tests:
JDK11:
x64-86: OpenJ9 - Personal/job/Pipeline-Build-Test-Personal/19087
zOS: Pipeline-Build-Test-Personal/19102
java8:
LoZ: jvm.29.personal/33327
zOS: jvm.29.personal/33326

@hzongaro hzongaro self-assigned this Nov 9, 2023
hzongaro
hzongaro approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

I've taken a quick look, and I think things look good, but I'm relying on the much more thorough reviews undertaken by others. Thanks!

@hzongaro
Copy link
Member

hzongaro commented Nov 9, 2023

This pull request has been thoroughly reviewed and tested. Merging.

@hzongaro hzongaro merged commit 9bc6e08 into eclipse:master Nov 9, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joransiu joransiu joransiu approved these changes

@keithc-ca keithc-ca keithc-ca approved these changes

@hzongaro hzongaro hzongaro approved these changes

@r30shah r30shah r30shah approved these changes

@charliegracie charliegracie Awaiting requested review from charliegracie

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl 0xdaryl is a code owner

Assignees

@hzongaro hzongaro

Labels
comp:utilities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ehsankianifar @r30shah @keithc-ca @hzongaro @joransiu