[Bug]: CLANG build of ArmGicLib failed due to ICC_SGI1R declaration
kuqin12 opened this issue · 11 comments
There has been report on failure of build due to new ICC_SGI1R declaration. Full report as below:
Hello!
I am still hitting issues with the ICC_SGI1R declaration using CLANG at the following location:
Here is a snippet of the error I'm observing:
2022-12-17T15:46:58.0647934Z INFO - /home/vsts/work/1/s/Build/SurfaceDuo1-AARCH64/DEBUG_CLANG38/AARCH64/ArmPkg/Drivers/ArmGic/ArmGicLib/OUTPUT/GicV3/AArch64/ArmGicV3.iiii:66:13: error: expected writable system register or pstate
2022-12-17T15:46:58.0649056Z INFO - msr ICC_SGI1R, x0
2022-12-17T15:46:58.0652365Z INFO - ^
2022-12-17T15:46:58.0653314Z INFO - make: *** [GNUmakefile:345: /home/vsts/work/1/s/Build/SurfaceDuo1-AARCH64/DEBUG_CLANG38/AARCH64/ArmPkg/Drivers/ArmGic/ArmGicLib/OUTPUT/GicV3/AArch64/ArmGicV3.obj] Error 1
2022-12-17T15:46:58.0653913Z INFO -
2022-12-17T15:46:58.0654191Z INFO -
2022-12-17T15:46:58.0654485Z INFO - build.py...
2022-12-17T15:46:58.0654857Z INFO - : error 7000: Failed to execute command
2022-12-17T15:46:58.0655440Z INFO - m a k e t b u i l d [/home/vsts/work/1/s/Build/SurfaceDuo1-AARCH64/DEBUG_CLANG38/AARCH64/ArmPkg/Drivers/ArmGic/ArmGicLib]
2022-12-17T15:46:58.5874992Z INFO -
2022-12-17T15:46:58.5875736Z INFO -
2022-12-17T15:46:58.5876142Z INFO - build.py...
2022-12-17T15:46:58.5876619Z INFO - : error F002: Failed to build module
2022-12-17T15:46:58.5877268Z INFO - /home/vsts/work/1/s/Silicon/ARM/TIANO/ArmPkg/Drivers/ArmGic/ArmGicLib.inf [AARCH64, CLANG38, DEBUG]
2022-12-17T15:46:58.5877809Z INFO -
2022-12-17T15:46:58.5878197Z INFO - - Failed -
2022-12-17T15:46:58.5878678Z INFO - Build end time: 15:46:58, Dec.17 2022
2022-12-17T15:46:58.5879168Z INFO - Build total time: 00:00:23
It appears the issue was fixed only in the masm file and not in the .S file as well.
Currently I'm working around the issue by defining a new define like so:
*_*_*_PP_FLAGS = -DICC_SGI1R=ICC_SGI1R_EL1
Originally posted by @gus33000 in #38 (comment)
I didn't see a problem in local GCC builds. If this is only a problem on CLANG, that's expected since it is not tested in CI. Otherwise, we need to make sure this can be caught in CI as part of the fix.
This issue has been automatically marked as stale because it has not had activity in 45 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.
This issue has been automatically been closed because it did not have any activity in 45 days and no follow up within 7 days after being marked stale. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had activity in 45 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.
Right now I'm still working around this issue by using a define in the command line arguments, but I assume it would be better here to not just correct the name used in the code but also introduce a clang ci?
Would be great if we could have newer clang versions supported in the process, I saw some work recently around getting CLANGPDB to work (would also help provide compiles under Windows itself for ARM64).
If it's just a define rename I can provide a PR fine for this. Would any other toolchain be impacted by such a rename? Does GCC support both names?
I was under the impression looking at the code above anyway gcc would get these redefined.
Slightly offtopic but there are other issues in general about some pip packages not working under Linux ARM64 either to compile here but that would be for an entirely different issue on its own.
Sorry for the delay. The support of CLANGPDB for ARM build has other issues at this point, which is why there is nothing as such yet. But we do want to support that properly in the future.
I do not think that renaming the definition will cause any issue, would you mind creating a PR? Otherwise, I will get to it later today.
On the other hand, I would expect that building on Linux ARM will come before the CLANGPDB support because this is more in our control.
Sure I will test this now
Works fine here, (builds fine and boots fine) with the ICC_SGI1R_EL1 workaround I previously used removed. Thanks!
Works fine here, (builds fine and boots fine) with the ICC_SGI1R_EL1 workaround I previously used removed. Thanks!
Thanks a lot for your verification. I will merge the PR once people sign it off.
The change is merged. Closing this ticket. Will try to upstream this entire SGI function change to edk2 with the updated version shortly.