[PATCH] D139092: [LLD][ELF] Cortex-M Security Extensions (CMSE) Support
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 9 15:00:31 PST 2023
MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.
Thanks! LGTM from my review. It'd be good to have a review from a domain expert.
I am going to be out for the next 4 weeks, and will likely have little time reading patches. The patch looks good to land when a domain expert thinks it's fine.
================
Comment at: lld/ELF/Arch/ARM.cpp:1005
+ // If input object build attributes do not support CMSE, error and disable
+ // further scanning for <sym>, acle_se_<sym> pairs
+ if (!config->armCMSESupport) {
----------------
================
Comment at: lld/ELF/InputFiles.cpp:308
+void elf::parseArmCMSEImportLib(std::optional<InputFile *> file) {
+ invokeELFT(doParseArmCMSEImportLib, file);
+}
----------------
MaskRay wrote:
> Place the `if (file)` check in the caller and use `InputFile &file` instead of `std::optional`.
My comment may be unclear.
`if (armCmseImpLib) parseArmCMSEImportLib(*armCmseImpLib);`, then this function will not need a `std::optional` argument.
================
Comment at: lld/ELF/InputFiles.h:51
+// Add symbols in File to the symbol table.
+void parseArmCMSEImportLib(std::optional<InputFile *> file);
----------------
Did you mean: Parse symbols in an Arm CMSE import library?
Or perhaps just remove the comment since the function name is self-explanatory.
================
Comment at: lld/ELF/Options.td:92
+defm out_implib: EEq<"out-implib",
+ "Output the CMSE secure code import library to the location specified (required when creating "
+ "a CMSE secure image)">,
----------------
`to the location specified` => `to <file>`.
The metasyntactic name `<file>` can be used to simplify and clarify the help message.
Perhaps @peter.smith has some specific suggestions here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139092/new/
https://reviews.llvm.org/D139092
More information about the llvm-commits
mailing list