[PATCH] D153430: Warn on invalid Arm or AArch64 baremetal target triple

peter.smith added a comment. I can confirm that we have seen several users of the LLVM Embedded Toolchain for Arm that work on both AArch64 and Arm make this mistake as it is easy to just alter an example triple by substituting the first element. Given that this is a warning, invalid is perhaps

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

peter.smith added a comment. My initial reactions to seeing Invalid target triple aarch64-none-eabi; try aarch64-none-elf [*] were: - Why is it invalid? - I assumed it was an error message, and was about to write a comment until I saw it was a warning. [X] now did you mean (thanks for making t

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

peter.smith added a comment. > clang: warning: mismatch between architecture and environment in target > triple 'aarch64-none-eabi'; did you mean 'aarch64-none-elf'? > [-Winvalid-command-line-argument] That looks good to me. Would be happy with that. Repository: rG LLVM Github Monorepo CHA

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Thanks for the update! LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153430/new/ https://reviews.llvm.org/D153430 _

[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags

peter.smith added a comment. Approach looks good to me. Some suggestions, mostly around documenting the interface. Comment at: clang/include/clang/Driver/Multilib.h:56 /// This is enforced with an assert in the constructor. Multilib(StringRef GCCSuffix = {}, StringRef OS

[PATCH] D151308: -fsanitize=function: fix alignment fault on Arm targets.

peter.smith added a comment. This looks good to me. Will be worth waiting for a day to give the US time zone time to leave any comments. I note that this is also broken in -fsanitize=kcfi [*] (https://reviews.llvm.org/D135411) although fixing that is a separate patch. Would you be able to rais

[PATCH] D151308: -fsanitize=function: fix alignment fault on Arm targets.

peter.smith added a comment. In D151308#4369828 , @MaskRay wrote: > In D151308#4367704 , @peter.smith > wrote: > >> This looks good to me. Will be worth waiting for a day to give the US time >> zone time to leav

[PATCH] D143587: [Docs] Multilib design

peter.smith added a comment. Thanks very much for writing this. Will be worth updating the RFC with a link as I think that this is an approachable place to comment on the format and selection model without the implementation detail. I'm happy with what has been written so far. My suggestions ar

[PATCH] D142893: [NFC] Class for building MultilibSet

peter.smith added a comment. Thanks for the updates, I'm happy with the changes and don't have any more comments. Happy to give my implicit approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142893/new/ https://reviews.llvm.org/D142893 _

[PATCH] D142905: Change multilib selection algorithm

peter.smith added a comment. Thanks for the updates, I'm happy with the changes and don't have any more comments. Happy to give my implicit approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142905/new/ https://reviews.llvm.org/D142905 _

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

peter.smith added a comment. Looks good to me. Some small suggestions around execute-only as I expect this to be a useful multilib variant. Comment at: clang/lib/Driver/ToolChain.cpp:198 unsigned FPUKind = llvm::ARM::FK_INVALID; - tools::arm::getARMTargetFeatures(D, Triple

[PATCH] D143059: [NFC] Enable selecting multiple multilibs

peter.smith added a comment. A couple of small suggestions but otherwise looks good to me. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:233 + std::string Result = computeBaseSysRoot(getDriver(), getTriple()); + if (!SelectedMultilibs.empty()) { +Result += Selecte

[PATCH] D143587: [Docs] Multilib design

peter.smith added a comment. Thanks for the update, one nit in the language, otherwise looks good to me. It is a good reflection of the implementation. Comment at: clang/docs/Multilib.rst:250 + +The API need only multilib selection based on only a limited set of command +line

[PATCH] D142933: Add -print-multi-selection-flags argument

peter.smith added a comment. Thanks for the updates, changes look good to me. I think that we can get away without trying to break the floating point options into flags, at least for how floating point units are available in hardware today. Comment at: clang/lib/Driver/ToolCh

[PATCH] D143587: [Docs] Multilib design

peter.smith added inline comments. Comment at: clang/docs/Multilib.rst:125 +``-fno-exceptions`` multilib variant need only contain C++ libraries. + +Stability Although implicit in the mechanism, is it worth highlighting that layered multib.yaml authors will need

[PATCH] D143075: BareMetal ToolChain multilib layering

peter.smith added a comment. No comments on the implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143075/new/ https://reviews.llvm.org/D143075 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D143763: [Clang] Add clangMinimumVersion to multilib.yaml

peter.smith added a comment. No objections to MaskRay's suggestion to merge with an earlier patch. I've made some suggestions to make the error messages be a bit more specific about what is wrong. Comment at: clang/lib/Driver/Multilib.cpp:185 +if (M.ClangMinimumVersion.em

[PATCH] D149946: [LoongArch] Define `ual` feature and override `allowsMisalignedMemoryAccesses`

peter.smith added a comment. No objections for moving out m_arm_Features_Group or adding the alias. It looks like that is currently unused i.e. no driver filters out the m_arm_Features_Group. I can't comment on the LoongArch specific parts of the patch. I think you may want to note in the help

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

peter.smith added a comment. Should `HANDLER(__ubsan_handle_function_type_mismatch,"function")` be added to ubsan_minimal_runtime if this is supported in the minimal runtime? Comment at: clang/lib/CodeGen/CGExpr.cpp:5382 + getPointerAlign()); llvm::Value *Calle

[PATCH] D148827: -fsanitize=function: support C

peter.smith added a comment. I agree with the reasoning. Can you update the documentation in clang/docs/UndefinedBehaviorSanitizer.rst to include C and state the known limitation. After D148573 it says: - ``-fsanitize=function``: Indirect call of a function

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

peter.smith added inline comments. Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92 ; CHECK-NEXT: .Ltmp{{.*}}: ; CHECK-NEXT: nop ; CHECK-NEXT: .word 3238382334 // 0xc105cafe samitolvanen wrote: > peter.smith wrote: > > Assuming t

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

peter.smith added inline comments. Comment at: clang/test/CodeGenCXX/catch-undef-behavior.cpp:408 // RTTI pointer check + // CHECK: [[CalleeTypeHashPtr:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, i32 }>* [[PTR]], i32 -1, i32 1 MaskRay wrote: > peter.smit

[PATCH] D148665: Change -fsanitize=function to place two words before the function entry

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Apologies for the delay LGTM. I think there is a case for setting up the signature to be target specific, but that could in theory be done on demand when a target adds a clashing ins

[PATCH] D148573: Port -fsanitize=function to AArch64

peter.smith added a comment. As it stands I think this may have problems with -mbranch-protection. In that case we'll need a `BTI c` to be the target of the indirect branch. I'm guessing something like: _Z3funv BTI C ; In hint space B . + 8 .word .L__llvm_rtti_proxy-_Z3funv Otherwise

[PATCH] D148573: Port -fsanitize=function to AArch64

peter.smith added a comment. One other small thing. The docs page https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html would need the supported architectures updating: -fsanitize=function: Indirect call of a function through a function pointer of the wrong type (Darwin/Linux, C++ and

[PATCH] D148573: Port -fsanitize=function to AArch64

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. With moving the signature before the function entry this looks good to me. I'm not so familiar with the code in https://reviews.llvm.org/D148665 would ideally find someone a bit more

[PATCH] D149458: [Driver] Pass --target2= to linker from baremetal toolchain

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM. This is consistent with the target2 support that I added to LLD several years ago in https://reviews.llvm.org/D25684 will be good to give some time for other reviewers to add a

[PATCH] D148665: Change -fsanitize=function to place two words before the function entry

peter.smith added a comment. My apologies for not responding. If I've got this right there are 4 related patches: D148573 (approved) D148785 Use type hashes rather than RTTI D148827 support C

[PATCH] D34513: [NFC] Update to account for DiagnosticRenderer use of FullSourceLoc

This revision was automatically updated to reflect the committed changes. Closed by commit rL306385: [NFC] Update to account for DiagnosticRenderer use of FullSourceLoc (authored by psmith). Changed prior to commit: https://reviews.llvm.org/D34513?vs=103570&id=104116#toc Repository: rL LLVM

[PATCH] D35538: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp

peter.smith created this revision. Herald added subscribers: kristof.beyls, aemerson. The ARM Runtime ABI document (IHI0043) defines the AEABI floating point helper functions in section 4.1 Floating-point library. These functions always use the base PCS (soft-fp). However helper functions define

[PATCH] D35538: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp

peter.smith added a comment. In https://reviews.llvm.org/D35538#814008, @compnerd wrote: > Can you please add a test that shows that the AEABI functions are not given > the wrong CC? Also, can you show that if someone also passes `-meabi=gnu` > with a VFP target, that we still annotate the fun

[PATCH] D35538: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp

peter.smith updated this revision to Diff 108092. peter.smith added a comment. Herald added a subscriber: javed.absar. I've added a clang test that will check that none of the floating point helper functions defined in the Runtime ABI for the ARM Architecture are directly called by clang. Given

[PATCH] D35538: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp

peter.smith updated this revision to Diff 108245. peter.smith added a comment. Thanks both for the comments, I've added the missing punctuation and have expanded the tests so that we anchor the check to the function and check for the operation that will be lowered by the back-end. https://revi

[PATCH] D35538: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp

peter.smith added a comment. I've created https://reviews.llvm.org/D35904 to cover adding a test for LLVMs use of the runtime helper functions. https://reviews.llvm.org/D35538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D35538: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp

This revision was automatically updated to reflect the committed changes. Closed by commit rL309257: [CodeGen][ARM] ARM runtime helper functions are not always soft-fp (authored by psmith). Changed prior to commit: https://reviews.llvm.org/D35538?vs=108245&id=108436#toc Repository: rL LLVM

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

peter.smith added a comment. I've no comments on the code in D101872 , and D10873 they look reasonable to me. I guess it is down to whether this is the right thing to do or not. Just to check my understanding: - Clang default

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

peter.smith added a comment. In D101873#2741299 , @MaskRay wrote: > https://gist.github.com/MaskRay/2d4dfcfc897341163f734afb59f689c6 has more > information about -fno-semantic-interposition. > >> Can Clang default to -fno-semantic-interposition? > > I th

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

peter.smith added a comment. Thanks for the update. With the clarification that this isn't breaking aarch64 long range thunks now, and we are not considering Arm then I'm happy for this to happen if the user opts in with -fno-semantic-interposition. I think the longer term question, outside of

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM as this is opt in with a command line option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101873/new/ https://reviews.llvm.org/

[PATCH] D100372: [Clang][ARM] Define __VFP_FP__ macro unconditionally

peter.smith accepted this revision. peter.smith added reviewers: compnerd, rengolin. peter.smith added a comment. This revision is now accepted and ready to land. I think this is the right thing to do. GCC changed to unconditionally set the macro with https://gcc.gnu.org/legacy-ml/gcc-patches/201

[PATCH] D111134: Add basic aarch64-none-elf bare metal driver.

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM thanks for the update. This looks like it follows the same format as the other -none-elf toolchains, and AArch64 can benefit from the bare-metal driver for easier access to LLD.

[PATCH] D45961: [MC] Add MCSubtargetInfo to MCAlignFragment

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5e71839f7793: [MC] Add MCSubtargetInfo to MCAlignFragment (authored by psmith). Herald added a project: clang. Herald added a subscriber: cfe-commits

<    1   2