[PATCH] D155808: [clang][driver] Missing the condition in IsARMBigEndain function.

peter.smith added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:39 // normalized triple so we must handle the flag here. bool arm::isARMBigEndian(const llvm::Triple , const ArgList ) { + if ((Triple.getArch() == llvm::Triple::arm || Is

[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] 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

[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

[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

[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

[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

[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

[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: >

[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

[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

[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

[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

[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

[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

[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

[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 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] 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

[PATCH] D143587: [Docs] Multilib design

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion that can be

[PATCH] D143075: BareMetal ToolChain multilib layering

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

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

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

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

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

[PATCH] D142933: Add -print-multi-selection-tags-experimental option

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

[PATCH] D142932: Multilib YAML parsing

peter.smith added a comment. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left some comments that can be applied prior to commit if you want to take them up. Comment at:

[PATCH] D145567: [Driver] Rename multilib flags to tags

peter.smith accepted this revision. peter.smith added a comment. Actually click the button this time to set approval, see previous comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145567/new/ https://reviews.llvm.org/D145567

[PATCH] D145567: [Driver] Rename multilib flags to tags

peter.smith added a comment. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. From looking at the source code alone - assuming that most people would not track down the commit message/review for extra

[PATCH] D142905: [Driver] Change multilib selection algorithm

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion for a comment,

[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

[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

[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

[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:

[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] 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 +=

[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,

[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] 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] 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

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

peter.smith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4162 def print_multi_lib : Flag<["-", "--"], "print-multi-lib">; +def print_multi_selection_flags : Flag<["-", "--"], "print-multi-selection-flags-experimental">, + HelpText<"Print the flags

[PATCH] D142905: Change multilib selection algorithm

peter.smith added a comment. A couple of small stylisitic suggestions. Comment at: clang/include/clang/Driver/Multilib.h:31 public: - using flags_list = std::vector; + using flags_list = std::set; + using arg_list = std::vector; would flags_set be a better

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

peter.smith added a comment. Looks good so far. Some stylistic suggestions. Comment at: clang/include/clang/Driver/Multilib.h:25 namespace driver { /// This corresponds to a single GCC Multilib, or a segment of one controlled It took a bit of reading to

[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

peter.smith added a comment. I've got about as far as the clang changes. As I mentioned in Discourse I'm not familiar enough in this area to give good feedback on the implementation, most if not all my comments fall under the bike shedding category. Will need to leave the high-level feedback

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM. GCC no longer supports Arm architecture prior to v4 so it is likely the alternative of adding support for v3 is not worth it. The only Arm machines running v2 are likely to be

[PATCH] D114124: [clang][ARM] only check -mtp=cp15 for non-asm sources

peter.smith added a comment. LGTM too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114124/new/ https://reviews.llvm.org/D114124 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

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

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

peter.smith added a comment. Apologies, missed a couple of small things out. Otherwise looks good to me. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150 +// The backend does not have support for hard thread pointers when targeting +// Thumb1.

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

peter.smith added a comment. I've made a suggestion to disallow v8-m.baseline (does not have Thumb 2 but has number > 7) and to simplify the expression. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155 + llvm::ARM::ArchKind AK =

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

peter.smith added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155 + llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName()); + return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 || + (Ver == 6 && Triple.isARM());

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

peter.smith added a comment. Apologies had to go on a bit of a dive through the documentation. I've put some comments inline and some links to other documents that may help. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150 +// The backend does not have support for

[PATCH] D112768: [ARM] implement support for TLS register based stack protector

peter.smith added a reviewer: ostannard. peter.smith added a comment. Adding ostannard to reviewers list. I'm going to be on vacation next week and Oliver is more familiar with this area than I am. To prevent the option in Clang for targets that don't support Thumb2 it may be worth looking

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

peter.smith added inline comments. Comment at: clang/test/CodeGen/arm-tphard.c:10 +} + nickdesaulniers wrote: > ardb wrote: > > nickdesaulniers wrote: > > > Let's make this a test under llvm/test/CodeGen/, using IR: > > > ``` > > > ; RUN: llc

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM as this as CP15 can be used on architecture v6k and above, which maps to IsThumb2. As an aside from this patch, the Arm state could be considered too permissive as it will

[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

[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:

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

peter.smith added a comment. Doing this on 32-bit Arm would make me nervous as STT_FUNC symbols encode the state of Arm/Thumb in the bottom bit, but STT_NOTYPE symbols do not. In principle it could be done but extra care would have to be taken to make sure no state changes were required. For

[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/

[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

[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

[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

[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

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

psmith added a comment. Radio silence so far; I think no news is good news applies in this case. I'm happy to say no objections. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91760/new/ https://reviews.llvm.org/D91760

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

psmith added a comment. Personally I'm in favour of clang and gcc behaving the same way unless there is a good reason not to. I've shared the review internally to see if anyone has any concerns. May be worth informing the clang built linux community to see if they will need to make any

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith added a comment. I agree it's not what I'd like. I'm not sure how to react to MaskRays comment. The top of the ClangCommandLineReference.rst says: --- NOTE: This file is automatically generated by running clang-tblgen

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

This revision was automatically updated to reflect the committed changes. psmith marked an inline comment as done. Closed by commit rG839d974ee0e4: [DOCS] Add more detail to stack protector documentation (authored by psmith). Herald added a project: clang. Repository: rG LLVM Github Monorepo

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith marked an inline comment as done. psmith added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2139 -Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic which considers functions vulnerable if they

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith updated this revision to Diff 283201. psmith added a comment. Herald added a subscriber: dang. uploaded diff with both Options.td and ClangCommandLineReference.rst CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85239/new/ https://reviews.llvm.org/D85239 Files:

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith added a comment. In D85239#2194604 , @MaskRay wrote: > This file is auto generated. The real documentation should go to > `clang/include/clang/Driver/Options.td` Thanks for pointing that out! I regenerated the ClangCommandLineReference.rst from

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith created this revision. psmith added reviewers: jfb, Shayne, emaste, kristof.beyls, mattdr, ojhunt, probinson, reames, serge-sans-paille, dim. Herald added a subscriber: dexonsmith. psmith requested review of this revision. The Clang -fstack-protector documentation mentions what functions

[PATCH] D80828: [Clang][A32/T32][Linux] -O1 implies -fomit-frame-pointer

psmith accepted this revision. psmith added a comment. LGTM from an Arm person now that the Android changes have been made. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:22 #include "InputInfo.h" +#include "MSP430.h" #include "PS4CPU.h" nickdesaulniers

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

psmith added a comment. FWIW I'm happy for Clang to match GCC behaviour for Linux (non-Android) targets with respect to the frame pointer. Outside of Android I would expect most developers of linux applications to build with both GCC and clang so they should already have the

[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

psmith added a comment. One question I can't answer and I think would need wider review, is whether this is type of material (common options for specific CPUs) is suited for the Clang Documentation or whether it would be better hosted by Arm itself, for example on developer.arm.com? I think

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

psmith accepted this revision. psmith added a comment. This revision is now accepted and ready to land. That wording looks good to me. I've accepted the change, but worth waiting a day or so to see if there are any objections or suggestions. CHANGES SINCE LAST ACTION

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

psmith accepted this revision. psmith added a comment. This revision is now accepted and ready to land. Thanks for the update, looks good to me. The BareMetal driver tests are better than the location I suggested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73904/new/

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

peter.smith added a comment. I think this is the right thing to do. According to https://sourceware.org/binutils/docs/ld/Options.html#Options Add the archive or object file specified by namespec to the list of files to link. This option may be used any number of times. If namespec is of the

[PATCH] D73865: [CodeGenModule] Assume dso_local for -fpic -fno-semantic-interposition

peter.smith added a comment. If I've understood correctly this would make LLVM more aggressive for PIC relocation models, but perhaps more honest in an inter procedural optimisation context? The code changes look fine to me, I'm wondering if we've discussed this widely enough with the

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. In D7#1836703 , @MaskRay wrote: > In D7#1836669 , @hans wrote: > > > In D7#1836643 , @peter.smith > > wrote: > > > > > >> If the

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. >> @peter.smith @nickdesaulniers What do you think? > > Revert on the 10.0 release sounds reasonable to me. That would prevent the > kernel from enabling the option and would prevent the build failure. I should have been clearer, apologies; we're not blocked by the

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. >> If the patchable functions is intended for clang-10 we'll need to make sure >> any fix is merged to clang-10. > > This commit was made before release/10.x branch. Maybe the easiest thing is > to revert the driver change in release/10.x (CC @hans), before we had

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. Although this particular commit will not be at fault, it is the option that enables the feature which is the earliest I can bisect the fault to. There are 3 files in linux that assert fail on the Implement the 'patchable-function attribute'. The files in question

[PATCH] D72959: Relative VTables ABI on Fuchsia

peter.smith added a comment. In D72959#1835368 , @rjmccall wrote: > There's two independently-useful things here for the relocation, right? > First, it's useful to be able to express a relocation to a symbol that has > the semantics of a particular

[PATCH] D72959: Relative VTables ABI on Fuchsia

peter.smith added a comment. In D72959#1832011 , @pcc wrote: > > On Aarch64, right now only CALL26 and JUMP26 instructions generate PLT > > relocations, so we manifest them with stubs that are just jumps to the > > original function. > > I think it

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

peter.smith added a comment. It is also failing on the other 32-bit arm bots, example http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp Looking at the failure // CHECK-SAME: @_ZThn8_N1AD1Ev and the

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

peter.smith added a comment. Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d , I've used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so you should show up as the author of the patch in the

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

This revision was automatically updated to reflect the committed changes. Closed by commit rG10c11e4e2d05: This option allows selecting the TLS size in the local exec TLS model, which is… (authored by kawashima-fj, committed by peter.smith). Herald added a subscriber: cfe-commits. Repository:

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Thanks for the update. That looks good to me. Will be good to wait for a day before committing to give US timezone a chance to object. Repository: rG LLVM Github Monorepo

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

peter.smith added a comment. After retesting on a failing example and experimenting a bit, I think that we should only preserve +crypto with -fno-integrated-as. It would also be good to mention D66018 and maybe add the original author as a reviewer.

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've not seen any objections so I've approved LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65000/new/ https://reviews.llvm.org/D65000

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith added a comment. test case missing A8 aside this looks ok to me. Would like to see if there are any comments from the Pacific time zone. Comment at: test/CodeGen/ARM/exception-alignment.cpp:8 +// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16 +#include +

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith added a comment. Thanks for the update. Will be worth adding some reviewers from Apple to see if this change should be IsAAPCS only. I've no more further comments myself besides a small nit on style. Comment at: lib/Basic/Targets/ARM.cpp:331 + // but

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith added reviewers: srhines, danalbert. peter.smith added a comment. I think that this may not apply for Android as AFAIK their ABI still requires 128-bit alignment in some cases. Adding some more reviewers from Android. Comment at: lib/Basic/Targets/ARM.cpp:311

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

peter.smith added a comment. FWIW I think this looks reasonable. The ARM equivalent uses bitfields such as unsigned CRC : 1; unsigned Crypto : 1; unsigned DSP : 1; unsigned Unaligned : 1; unsigned DotProd : 1; Which would make more sense than using unsigned for each individual field.

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

peter.smith added a comment. In D60472#1461336 , @manojgupta wrote: > The motivation for this change is to make "crypto" setting an additive option > e.g. like "-mavx" used in many media packages. Some packages in Chrome want > to enable crypto

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

peter.smith added a comment. I'm not in favour of adding AArch64 support to -mcrypto -mnocrypto for a few reasons: - Arm are trying to keep the options for controlling target features as consistent as possible with GCC and this option isn't supported in GCC

[PATCH] D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library.

peter.smith added a comment. I've no objections to adding the command line as a Darwin only option. Implementation looks fine to me although I've not got any prior experience with Darwin. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58320/new/

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

peter.smith added a comment. Thanks for the update. To summarise I'd like to keep the integrated and non-integrated assembler in synch for Linux like Android, even at the risk of adding an fpu when it isn't needed. I think that given how difficult it is to not use NEON, I think the

[PATCH] D58429: [CodeGen] Enable the complex-math test for arm

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the fix. My apologies for missing that at the time, it looks like a cut and paste error on my part. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

peter.smith added a comment. My main concern is that this changes the default behaviour for arm-linux-gnueabi and arm-linux-gnueabihf targets. I've put some suggestions on what I think the behaviour should be. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277 + } +

[PATCH] D58320: [Darwin] Introduce a new flag, -flink-builtins-rt that forces linking of the builtins library.

peter.smith added a comment. The implementation changes in the Darwin toolchain look fine to me, although with respect to the command line option I think Petr Hosek's message on cfe-dev is interesting: > GCC implements -nolibc which could be used to achieve the same effect when > combined

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

peter.smith added a comment. I think we need to be very careful here. If I've understood this correctly then if this functionality is used for anything critical then a manually supplied target will be needed when doing cross-linking. For example my host LLD is x86_64, is just called ld.lld and

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

peter.smith added a comment. In D55029#1313120 , @ruiu wrote: > LGTM. Please commit. > > Peter, I wonder if you are fine with the default 64KiB page size with lld, > especially given that lld always round up the text segment size to the > maximum page

  1   2   >