Skip to content

[flang] Don't associate pointers with zero sized storage targets #155867

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
Merged
merged 5 commits into from
Aug 28, 2025
Merged

[flang] Don't associate pointers with zero sized storage targets #155867

merged 5 commits into from
Aug 28, 2025

Conversation

@eugeneepshteyn
Copy link
Contributor

@eugeneepshteyn eugeneepshteyn commented Aug 28, 2025

Fixes #155481

@eugeneepshteyn eugeneepshteyn marked this pull request as ready for review August 28, 2025 15:56
klausler
klausler reviewed Aug 28, 2025
View reviewed changes
flang-rt/lib/runtime/pointer.cpp Outdated
void RTDEF(PointerAssociate)(Descriptor &pointer, const Descriptor &target) {
pointer = target;
pointer.raw().attribute = CFI_attribute_pointer;
if (target.ElementBytes() > 0) {
Copy link
Contributor

@klausler klausler Aug 28, 2025

Choose a reason for hiding this comment

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

This would leave the pointer unchanged when the target is an empty storage sequence, which would be wrong.

Copy link
Contributor

@DanielCChen DanielCChen Aug 28, 2025

Choose a reason for hiding this comment

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

Correct. The pointer object will still need to get the dynamic type and etc from the target even though it's association status is not associated with the zero-sized storage sequence.

DanielCChen
DanielCChen approved these changes Aug 28, 2025
View reviewed changes
Copy link
Contributor

@DanielCChen DanielCChen left a comment

Choose a reason for hiding this comment

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

LGTM.
It fixed all the test cases marked against this issue.
Thanks!

klausler
klausler reviewed Aug 28, 2025
View reviewed changes
Copy link
Contributor

@klausler klausler left a comment

Choose a reason for hiding this comment

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

Please run the llvm-test-suite before merging; I think that GNU handles this case incorrectly and probably confirms it with a test; if so, it'll need to be disabled.

flang-rt/lib/runtime/pointer.cpp Outdated
}
if (!target->raw().base_addr ||
(target->raw().type != CFI_type_struct && target->ElementBytes() == 0)) {
if (!target->raw().base_addr || target->ElementBytes() == 0) {
Copy link
Contributor

@klausler klausler Aug 28, 2025

Choose a reason for hiding this comment

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

what about the case of ElementBytes() != 0 but no elements?

@eugeneepshteyn
Copy link
Contributor Author

eugeneepshteyn commented Aug 28, 2025 •
edited
Loading

Please run the llvm-test-suite before merging; I think that GNU handles this case incorrectly and probably confirms it with a test; if so, it'll need to be disabled.

I did. These failures happen for me even without my change:

Failed Tests (2):
  test-suite :: Fortran/gfortran/regression/gfortran-regression-execute-regression__eoshift_bounds_1_f90.test
  test-suite :: Fortran/gfortran/regression/goacc/gfortran-regression-compile-regression__goacc__routine-3_f90.test

I think eoshift needs to run at -O0 and ACC test has issue generating LLVM module.

akuhlens
akuhlens approved these changes Aug 28, 2025
View reviewed changes
@eugeneepshteyn eugeneepshteyn merged commit 313b52f into llvm:main Aug 28, 2025
9 checks passed
@eugeneepshteyn eugeneepshteyn deleted the assoc-empty branch August 28, 2025 22:53
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 28, 2025

LLVM Buildbot has detected a new failure on builder clang-aarch64-sve-vls running on linaro-g3-01 while building flang-rt at step 14 "test-suite".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/10381

Here is the relevant piece of the build log for the reference 
Step 14 (test-suite) failure: test (failure)
******************** TEST 'test-suite :: Fortran/gfortran/regression/gfortran-regression-execute-regression__pointer_init_8_f90.test' FAILED ********************

/home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --redirect-input /dev/null --chdir /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/gfortran/regression/gfortran-regression-execute-regression__pointer_init_8_f90.wd --summary /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/gfortran/regression/Output/gfortran-regression-execute-regression__pointer_init_8_f90.test.time /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/gfortran/regression/gfortran-regression-execute-regression__pointer_init_8_f90

+ /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --redirect-input /dev/null --chdir /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/gfortran/regression/gfortran-regression-execute-regression__pointer_init_8_f90.wd --summary /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/gfortran/regression/Output/gfortran-regression-execute-regression__pointer_init_8_f90.test.time /home/tcwg-buildbot/worker/clang-aarch64-sve-vls/test/sandbox/build/Fortran/gfortran/regression/gfortran-regression-execute-regression__pointer_init_8_f90
Fortran STOP: code 1


********************

@eugeneepshteyn
Copy link
Contributor Author

eugeneepshteyn commented Aug 29, 2025

LLVM Buildbot has detected a new failure on builder clang-aarch64-sve-vls

For some reason this test doesn't run for x86, so it didn't fail for me. (I should start running llvm-test-suite on ARM as well.)

I'll disable the test, since gfortran's behavior is now different from flang's.

@eugeneepshteyn
Copy link
Contributor Author

eugeneepshteyn commented Aug 29, 2025

PR for llvm-test-suite: llvm/llvm-test-suite#279

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

Reviewers

@akuhlens akuhlens akuhlens approved these changes

@klausler klausler klausler left review comments

@DanielCChen DanielCChen DanielCChen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[flang] Incorrect result of ASSOCIATED when the pointer is associated with zero sized storage sequence

5 participants

@eugeneepshteyn @llvm-ci @akuhlens @DanielCChen @klausler