Time: questionable unconditional access of `PMCCNTR` under `__ARM_ARCH_7A__`

[zpl-c]

Time: questionable unconditional access of `PMCCNTR` under `__ARM_ARCH_7A__`

[zpl-zak]

Unfortunately, the ARM-related implementation of zpl_rdtsc is very flawed due to a lack of expertise in that particular architecture. I agree a refinement should be made to improve and better handle special conditions.

[helixhorned]

To me, the ARMv6-upward implementation seems perfectly fine to use on ARMv7 as well, but I assume there was an intention behind the special case for __ARM_ARCH_7A__? (And I furthermore assume that all code present in the codebase was actually executed at least once and found working, so that there exists at least one system on which the code in question ran through.)

@inlife, you probably had something in mind when you wrote it in commit 2a6e301?

[inlife]

Hey @helixhorned

Unfortunately, I can't say I remember any details about this specific macro sequence.

but then the preprocessor condition needs to be refined IMO.

I fully support the refinement, I assume you already have a great suggestion in mind?

[helixhorned]

I don't have a great suggestion but a humble one which I thought you might not like: namely removing the code under #elif defined(__ARM_ARCH_7A__) altogether. Now up in PR #46.

Maybe in the future, the original intention becomes apparent again (for example, maybe there is a variant of the Arm architecture that simply doesn't have access checks and selecting that architecture was the intent?) and then the code in question can be brought back again. In my opinion that's better than aborting with SIGILL. You decide, of course :).

[inlife]

Ok, I think that sounds quite reasonable 👍
Thank you!

[zpl-zak]

Changes are now present in version 10.8.6. Should we close this issue?

[helixhorned]

Changes are now present in version 10.8.6. Should we close this issue?

Cool with me! 👍

[zpl-zak]

Thanks for the contribution, I really appreciate it!

[helixhorned]

You're welcome!

Just a follow-up: I saw commit e426c2f which moved the status on the Raspberry Pi from '❔' to '✅'.

I did not mean to imply that I had exercised the functionality offered by ZPL in its
entirety. In fact, I did not get the tests to build. After all, there still seem to be a few
corners that are unimplemented on ARM:

$ make code/tests/tester
=> Building tester
In file included from code/zpl.h:717,
                 from code/tests/tester.c:2:
code/source/threading/atomic.c:344:6: error: #error TODO: Implement Atomics for this CPU
     #error TODO: Implement Atomics for this CPU
      ^~~~~
In file included from code/zpl.h:718,
                 from code/tests/tester.c:2:
code/source/threading/fence.c: In function ‘zpl_yield_thread’:
code/source/threading/fence.c:37:10: error: #error Unknown architecture
         #error Unknown architecture
          ^~~~~

 (^ Same for ‘zpl_mfence’, ‘zpl_sfence’ and ‘zpl_lfence’.)

code/source/threading/thread.c: In function ‘zpl_thread_current_id’:
code/source/threading/thread.c:111:10: error: #error Unsupported architecture for zpl_thread_current_id()
         #error Unsupported architecture for zpl_thread_current_id()
          ^~~~~
make: *** [Makefile:59: code/tests/tester] Error 1

[inlife]

Ah, I assumed the mentioned previously issue was the only thing that was supported on Raspberry Pi. And since it got resolved it seemed a good idea to mark it as is.

@helixhorned would you like to introduce the improvements to the Raspberry Pi side of things?