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

[flang] Do not leak intrinsics used by ISO_C_BINDING and ISO_FORTRAN_ENV #79006

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

mjklemm
Copy link
Contributor

@mjklemm mjklemm commented Jan 22, 2024

This resolves bug #78953. Intrinsics used by the MODULE definition are being declared PRIVATE, so that they do not leak into the namespace of the code that USEs the modules.

@mjklemm mjklemm self-assigned this Jan 22, 2024
@mjklemm mjklemm requested a review from klausler January 22, 2024 16:48
@llvmbot llvmbot added the flang Flang issues not falling into any other category label Jan 22, 2024
@@ -22,6 +22,12 @@ module iso_c_binding
c_sizeof => sizeof, &
operator(==), operator(/=)

implicit none

Copy link
Contributor

@rzurob rzurob Jan 22, 2024

Choose a reason for hiding this comment

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

It might be better to add the private statement on line 26 so that everything is private by default and then add explicit public statements for the things you want to be public. This way, future additions don't leak by default.

Copy link
Contributor Author

@mjklemm mjklemm Jan 22, 2024

Choose a reason for hiding this comment

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

Good point! Even though it's way more to change, I like the suggestion as it's future proof. Please have a look at the new commit.

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.

LGTM; thanks!

@mjklemm
Copy link
Contributor Author

mjklemm commented Jan 22, 2024

LGTM; thanks!

Please have another look. @rzurob Had a good suggestion to turn the logic around and make everything private by default.

flang/module/iso_c_binding.f90 Outdated Show resolved Hide resolved
Copy link
Contributor

@rzurob rzurob left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mjklemm mjklemm merged commit ab32a3c into llvm:main Jan 23, 2024
4 checks passed
kparzysz pushed a commit that referenced this pull request Feb 19, 2024
…bols (#80833)

This PR continues the work started with PR #79006, by setting visibility
in MODULES to PRIVATE by default and explicitly exporting only the
desired symbols. `omp_lib` needs more work, as it should maybe be
compiled from `omp_lib.f90` in
`openmp/runtime/src/incluce/omp_lib.f90.var` instead of simply using an
INCLUDE for `omp_lib.h`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants