Added macro calls to adjust external reference for Darwin/AARCH64 link compatibility. Deleted ".size" directive not accepted by llvm/Clang arm64 assembler. I do not know whether the .size directive is accepted, or required, by the PPC64, MIPS64, or RISCV64 assemblers as I have no way to test on those.
For CHERI-extended architectures (RISCV,MIPS,AArch64), we need .size to get sensible bounds on pointers. While we don't support any of this code yet, keeping existing .size directives would be good.
Yes, that’s how the code in lines 170-189 (for ARCH_X86) and 191-210 (for ARCH_X86_64) handle this, by assuming that “if not DARWIN then must be LINUX*”. Both those blocks are within an “if KMP_GOMP_COMPAT” (“support for unnamed common blocks”) lines 163-212. It isn’t clear to me that the .data (and .size) code fragment we’re discussing (1744-1754) should not also be within a KMP_GOMP_COMPAT guard and merged with 163-212. A cleaner alternative to guarding individual .size directives might be a “SIZE” macro that conditionally emits or ignores .size directives. I’m not hereby proposing such a reorganization, but the “#if” method has its limitations, and keeping the very similar .data creation code together would be more readable.
If we do simply guard the line 1752 .size, would it be “if not DARWIN”, or need we exclude other OS platforms?
I'm new at this and could use guidance. I have tested that simply guarding the ".size" with "#if ! KMP_OS_DARWIN" is sufficient to build and link on macOS Big Sur Beta. If I should make a fresh "diff" and link it into this discussion, I'm happy to do that. It leaves unanswered, for now, whether the ".size" is or is not compatible with some Linux variants but would certainly be a step forward.
.size should work on all ELF platforms. I'm not sure which platforms are supported here, so maybe !Darwin is sufficient.
Checking for __ELF__ might also be an option since that should be set by both clang and GCC ( https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html)
Please upload correct patch - there is no such file in the root of monorepo, so what's the omitted path to the file?
Also, subj could include the projects name tag (i'm guessing, it's [libomp]?)
Since the proposed change shows as "Accepted" on October 23, I do not know what more I am able to do.
Let me know please if I'm blocking something inadvertently.
+1.858.354.4391 Voice, SMS text, iMessage
I am sorry, my accept action was not formal enough.
What you can do now is:
- clone the monorepo;
- apply your changes to openmp/runtime/src/z_Linux_asm.S, please also address @arichardson's comment on #if/#ifdef;
- run diff, e.g: $ git diff -U 999999 > newdiff.diff
- update the review diff with newdiff.diff (of cause, feel free to chose whatever name you like for the file instead of newdiff).
Once the updated patch is approved, it needs to be landed. Do you have commit permission to the monorepo?
If not, you can ask somebody who has the permission to commit the patch for you (e.g. I can do this).
I guess several architecture like KMP_ARCH_PPC64, KMP_ARCH_MIPS64, and KMP_ARCH_RISCV64 doesn't have KMP_PREFIX_UNDERSCORE definition here. I may be wrong, though. I'm working on VE architecture which is not merged yet. It shows compile error here after this patch.