[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

smeenai added a comment. This looks great to me, thanks. @rjmccall should sign off on it though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108905/new/ https://reviews.llvm.org/D108905 ___

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

smeenai added a comment. I'd vote for something like `-fassume-nothrow-exception-dtor` or `-fenforce-nothrow-exception-dtor` depending on if the patch will also implement the enforcement mentioned in https://github.com/llvm/llvm-project/issues/57375#issuecomment-1230590204, but we can

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

smeenai added a comment. In D108905#4654403 , @ChuanqiXu wrote: > In D108905#4654393 , @smeenai wrote: > >> In D108905#2975712 , @rsmith wrote: >> >>> No decision as yet,

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

smeenai added a comment. Herald added subscribers: pmatos, asb. Herald added a project: All. In D108905#2975712 , @rsmith wrote: > No decision as yet, but so far it looks very likely that we'll settle on the > rule that exceptions cannot have

[PATCH] D140925: [CMake] Use Clang to infer the target triple

smeenai added inline comments. Comment at: runtimes/CMakeLists.txt:167 +if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE) + set(CXX_TARGET_TRIPLE ${CMAKE_CXX_COMPILER} --target=${LLVM_RUNTIME_TRIPLE} -print-target-triple) ldionne wrote: > Is there any

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. In D158476#4647652 , @MaskRay wrote: > nits Sorry, this came in right as I committed the diff. I pushed rG915ebb07dfc53486eccf0dc09b6411929a463e98 to

[PATCH] D158476: [driver] Search for compatible Android runtime directories

This revision was automatically updated to reflect the committed changes. Closed by commit rG58288c6c1214: [driver] Search for compatible Android runtime directories (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158476/new/

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

This revision was automatically updated to reflect the committed changes. Closed by commit rGb1e3cd1d7944: [driver] Conditionally include installed libc++ headers for Android (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

This revision was automatically updated to reflect the committed changes. Closed by commit rG1212d1b51125: [driver] Perform fallback target searches for stdlib (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159293/new/

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

smeenai added a comment. In D159292#4646811 , @rprichard wrote: > In D159292#4646096 , @smeenai wrote: > >> Ping. > > I asked a couple of other people at Google to take a look at the patches. > Someone should

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158476/new/ https://reviews.llvm.org/D158476 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

smeenai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159292/new/ https://reviews.llvm.org/D159292 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D159486: Fix build error in CI.

smeenai added a comment. I don't think pre-merge testing covers Mac, only Linux and Windows (which is presumably how the original breakage got in). If the test passes for you locally I'd just push it and monitor the bots afterwards. (That isn't general advice, but in this specific case, since

[PATCH] D159486: Fix build error in CI.

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. The `-triple x86_64-unknown-unknown` shouldn't be required after the regex change, but I'm fine either way. I'd recommend pushing this to fix the bots, and Aaron/Nico can do a post-commit

[PATCH] D159486: Fix build error in CI.

smeenai added a comment. Generally it's preferred to either push bot failure fixes directly or revert the original commit (also without review), to get bots unblocked as soon as possible :) From what I can see, the test failure is because `dso_local` isn't emitted on Mac. That seems

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. Ping. I added the warning and release note. I haven't made search paths be dumped in `-v` mode because I thought it could be pretty noisy (especially since you'll be doing three searches, for resource dir libraries, libc++ per-target headers, and non-resource dir

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

smeenai added a comment. In D159292#4632389 , @rprichard wrote: > This change looks like an improvement to me. The NDK currently puts the > libc++ headers into the sysroot, but putting it in the usual toolchain > include location seems better. Are you

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

smeenai updated this revision to Diff 555247. smeenai added a comment. Rebase to avoid pre-existing CI failure Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159293/new/ https://reviews.llvm.org/D159293 Files:

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

smeenai updated this revision to Diff 555246. smeenai added a comment. Rebase to avoid pre-existing CI failure Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159292/new/ https://reviews.llvm.org/D159292 Files: clang/lib/Driver/ToolChains/Gnu.cpp

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai updated this revision to Diff 555213. smeenai added a comment. Add warning and release note Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158476/new/ https://reviews.llvm.org/D158476 Files: clang/docs/ReleaseNotes.rst

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. In D158476#4632188 , @thakis wrote: > Works for me. How do you imagine the transition happening? Should we emit > some kind of warning if the old fallback is hit? That's a good call. I'll add that plus a release note.

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

smeenai added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:3108 +SmallString<128> TargetDir(Path); +llvm::sys::path::append(TargetDir, Target, "c++", Version); if (D.getVFS().exists(TargetDir)) rprichard wrote: > Will the

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a reviewer: glandium. smeenai added a comment. @thakis I'm inclined to leave the fallback for a triple without any version in place for now, since it seems like Mozilla might be relying on it too, and we can remove it once everyone's moved over to the new fallback mechanism. What

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai updated this revision to Diff 555086. smeenai added a comment. Kick off pre-merge checks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158476/new/ https://reviews.llvm.org/D158476 Files: clang/include/clang/Driver/ToolChain.h

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

smeenai updated this revision to Diff 555084. smeenai added a comment. Kick off pre-merge checks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159293/new/ https://reviews.llvm.org/D159293 Files: clang/include/clang/Driver/ToolChain.h

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

smeenai added a comment. In D158475#4626856 , @glandium wrote: > In D158475#4626636 , @smeenai wrote: > >> I'm halfway through an implementation of this, but I just realized that on >> Android, Clang never

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai updated this revision to Diff 555082. smeenai added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158476/new/ https://reviews.llvm.org/D158476 Files: clang/include/clang/Driver/ToolChain.h

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

smeenai created this revision. smeenai added reviewers: MaskRay, phosek, thakis, glandium. Herald added a subscriber: abrachet. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Searching for

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

smeenai created this revision. smeenai added reviewers: danalbert, srhines, pcc, phosek, thakis, MaskRay, glandium. Herald added a subscriber: danielkiss. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

smeenai added a comment. In D152495#4628877 , @hans wrote: > In D152495#4628870 , @goncharov > wrote: > >> due to this change we have a enourmous number of new warnings, on the other >> hand -Wunused-variable

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

smeenai added a comment. In D152495#4628028 , @goncharov wrote: > there is a number of unused vaiables in conditional loops that are firing > now, I have submitted > https://github.com/llvm/llvm-project/commit/74f4daef0412be33002bd4e24a30cb47d0187ecf

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

smeenai added a comment. In D158475#4623852 , @glandium wrote: > In D158475#4623842 , @smeenai wrote: > >> In D158475#4623471 , @glandium >> wrote: >> >>> This conflicts

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

smeenai added a comment. In D158475#4623471 , @glandium wrote: > This conflicts with D146664 It sounds like you want the same logic as D158476 to apply to the stdlib search as well

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb6a1473f97d3: [driver] Refactor getRuntimePaths. NFC (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. D158798 demonstrates a working Android runtimes setup with per-target runtime directories once this change is in place. It'll be nicer once D140925 lands, but it's workable even without that.

[PATCH] D158798: [RFC] Android runtimes build demonstration

smeenai added a comment. The manual `androideabi` normalization is only needed when building the runtimes, not when using them: $ for i in {21..32}; do printf '%s ' $i; bin/clang --print-file-name=libclang_rt.builtins.a -target armv7-none-linux-androideabi$i; done 21

[PATCH] D158798: [RFC] Android runtimes build demonstration

smeenai created this revision. smeenai added reviewers: danalbert, rprichard, srhines. Herald added a subscriber: danielkiss. Herald added a project: All. smeenai requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai updated this revision to Diff 553291. smeenai added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158476/new/ https://reviews.llvm.org/D158476 Files: clang/include/clang/Driver/ToolChain.h

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

smeenai updated this revision to Diff 553290. smeenai added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158475/new/ https://reviews.llvm.org/D158475 Files: clang/include/clang/Driver/ToolChain.h clang/lib/Driver/Driver.cpp

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. In D158476#4609243 , @srhines wrote: > This approach LGTM, assuming that https://reviews.llvm.org/D140925 lands as > well to ensure that the triple does use `androideabi`, since it would prevent > custom build setups from

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. In D158476#4608428 , @thakis wrote: > I think this is a useful feature, for the reasons mentioned on the thread. > > Since this is a superset of D115049 (... > right?), this should probably

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai added a comment. In D158476#4605360 , @MaskRay wrote: >> // Android target triples contain a target version. If we don't have >> libraries for the exact target version, we should fall back to the next >> newest version or a versionless path, if

[PATCH] D158476: [driver] Search for compatible Android runtime directories

smeenai created this revision. smeenai added reviewers: collinbaker, thakis, MaskRay, phosek, danalbert, srhines. Herald added subscribers: danielkiss, kristof.beyls. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber:

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

smeenai created this revision. smeenai added reviewers: collinbaker, thakis, phosek, MaskRay. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This used to be getRuntimePath till

[PATCH] D140925: [CMake] Use Clang to infer the target triple

smeenai added a comment. Herald added a subscriber: ekilmer. Herald added a project: libunwind. Herald added a reviewer: libunwind. I think we should land this. Clang has grown workarounds in the meantime, e.g.

[PATCH] D158218: [CMake] Deprecate DEFAULT_SYSROOT and GCC_INSTALL_PREFIX

smeenai added a comment. Conceptually this seems like a great direction to go in. We should just leave it up for a while so that anyone using these can comment on any potential issues. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158218/new/

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

smeenai added a comment. In D141918#4577930 , @jyknight wrote: > In D141918#4566838 , @smeenai wrote: > >> $ clang -std=c++20 -O2 -c crash.cpp >> cannot use musttail with unwindabort > > Thanks for the report. We

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

smeenai added a subscriber: mstorsjo. smeenai added inline comments. Comment at: clang/test/Driver/cl-options.c:567 -// This test was super sneaky: "/Z7" means "line-tables", but "-gdwarf" occurs -// later on the command line, so it should win. Interestingly the cc1 arguments

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

smeenai added inline comments. Comment at: clang/test/Driver/cl-options.c:567 -// This test was super sneaky: "/Z7" means "line-tables", but "-gdwarf" occurs -// later on the command line, so it should win. Interestingly the cc1 arguments -// came out right, but had wrong

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

smeenai added a comment. Thanks for the rebase! We're eager to try this for Meta's Android apps, where size is a priority and better `noexcept` codegen would be really helpful. I ran into a crash with coroutines though, which blocks me from fully evaluating the size difference made by these

[PATCH] D153623: [clang][Sema] Add fixit for scoped enum format error

smeenai added a comment. Sorry for the delayed comment here. The fix-it is convenient, but is it the best suggestion? It'll end up suggesting truncating the enum value instead of using the proper format specifier in https://godbolt.org/z/xdhrefG95, for example. More insidiously, the

[PATCH] D155081: Specify the developer policy around links to external resources

smeenai added inline comments. Comment at: llvm/docs/DeveloperPolicy.rst:359 + If the patch fixes a bug in GitHub Issues, we encourage adding + "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing + the issue in GitHub. If the patch has been reviewed,

[PATCH] D153989: [compiler-rt] Move crt into builtins

smeenai added a comment. In D153989#4465907 , @phosek wrote: > In D153989#4465759 , @smeenai wrote: > >> Sorry, it's been a week :D I assume that crt doesn't need the builtins to be >> available for its

[PATCH] D153989: [compiler-rt] Move crt into builtins

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. Sorry, it's been a week :D I assume that crt doesn't need the builtins to be available for its configure (the way the rest of compiler-rt does)? If so, this LGTM.

[PATCH] D153176: [Frontend] Remove ShowIncludesPretendHeader

This revision was automatically updated to reflect the committed changes. Closed by commit rG1df10f15807f: [Frontend] Remove ShowIncludesPretendHeader (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153176/new/

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

This revision was automatically updated to reflect the committed changes. Closed by commit rG67a11290df64: [Frontend] Dont output skipped includes from predefines (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153175/new/

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

smeenai added a comment. In D153175#4431923 , @hans wrote: >> -H is supposed to skip outputting headers from -include command line >> arguments, but -fshow-skipped-includes was outputting any skipped >> includes encountered via -include. > > I was thrown

[PATCH] D153176: [Frontend] Remove ShowIncludesPretendHeader

smeenai created this revision. smeenai added reviewers: hans, rnk, thakis. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. It hasn't been written to since https://reviews.llvm.org/D46652, so it was

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

smeenai created this revision. smeenai added reviewers: hans, rnk, thakis. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. `-H` is supposed to skip outputting headers from `-include` command line

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

smeenai added a comment. Makes sense, though @Amir should also take a look. Comment at: bolt/CMakeLists.txt:122 + +add_custom_target(bolt DEPENDS ${BOLT_DEPENDS}) +set_target_properties(bolt PROPERTIES FOLDER "BOLT") Where is BOLT_DEPENDS being set now? The

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

smeenai added a comment. I searched for `__clang_call_terminate` in the Clang test directory when I was working on this diff. Most tests were testing calls to it, not definitions. There were a couple of tests checking the definition, but they seemed pretty targeted (e.g.

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

This revision was automatically updated to reflect the committed changes. Closed by commit rG8f7b51e4ec09: [CodeGen] Make __clang_call_terminate have an unwind table entry (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

smeenai updated this revision to Diff 525429. smeenai added a comment. Call SetLLVMFunctionAttributesForDefinition instead Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151393/new/ https://reviews.llvm.org/D151393 Files:

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

smeenai created this revision. smeenai added reviewers: efriedma, jyknight, rnk, MaskRay. Herald added a subscriber: kristof.beyls. Herald added a project: All. smeenai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This enables debuggers

[PATCH] D148654: Modify BoundsSan to improve debuggability

smeenai added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:189 auto GetTrapBB = [](BuilderTy ) { -if (TrapBB && SingleTrapBB) - return TrapBB; - -Function *Fn = IRB.GetInsertBlock()->getParent(); -// FIXME: This debug

[PATCH] D148654: Modify BoundsSan to improve debuggability

smeenai added a reviewer: melver. smeenai added a comment. The patch should be uploaded with full context to make review easier. Adding another potential reviewer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148654/new/

[PATCH] D149504: [clang][CodeGenPGO] Don't use an invalid index when region counts disagree

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. Seems pretty reasonable to add a bounds check here. You should probably add a comment explaining the reasoning though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148654: Modify BoundsSan to improve debuggability

smeenai added a comment. Thinking about this a bit more, should the trap not have an associated stack trace that can be symbolicated to tell you which line of code was crashing? If the issue is that multiple traps can get folded together, the `nomerge` attribute (D78659

[PATCH] D148654: Modify BoundsSan to improve debuggability

smeenai added a comment. You should upload this with full context and add some test cases :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148654/new/ https://reviews.llvm.org/D148654 ___ cfe-commits

[PATCH] D147764: Fix the two gmoules-prefered-name-* tests

smeenai added reviewers: Michael137, dblaikie, aprantl. smeenai added a comment. Thanks! I'm not super familiar with this, so I'm adding the original author and reviewers to confirm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147764/new/

[PATCH] D145228: [clangd] Add clangd headers to install targets

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145228/new/ https://reviews.llvm.org/D145228

[PATCH] D144603: Add option to disable compiler launcher on external projects

smeenai added a comment. In D144603#4162192 , @haowei wrote: > In D144603#4162048 , @smeenai wrote: > >> Hmm, what cache key does ccache use for compilers? Is it the `--version` >> output string, the path, or

[PATCH] D144603: Add option to disable compiler launcher on external projects

smeenai added a comment. Hmm, what cache key does ccache use for compilers? Is it the `--version` output string, the path, or some combination? If the path is involved then I don't see any value in using ccache for configuring anything past stage1, since the compiler will (presumably) be

[PATCH] D140925: [CMake] Use Clang to infer the target triple

smeenai added a comment. Ping @ldionne, would you be able to take a look at this soon (or are you okay waiving the libc++ blocking requirement for it)? This is really useful for Android armv7, where the triple is traditionally spelled armv7-none-linux-androideabi but normalized to

[PATCH] D140925: [CMake] Use Clang to infer the target triple

smeenai accepted this revision. smeenai added a comment. @tamas' suggestion would be a good change to make IMO, but I think it's outside the scope of this patch, and the patch as-is improves the status quo, so LGTM. Is there any way to share the normalization logic between the two locations, or

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

smeenai added a comment. In D127812#4031849 , @ilinpv wrote: > In D127812#4031313 , @smeenai wrote: > >> We can use `-mno-fmv` to avoid that dependency, right? We're interested in >> using that for our own code

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

smeenai added a comment. In D127812#4031101 , @ilinpv wrote: > In D127812#4030881 , @smeenai wrote: > >> We're not actually using multi-versioning anywhere, but we're still paying >> the size cost for it as a

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

smeenai added subscribers: yozhu, lanza, smeenai. smeenai added a comment. I'm investigating a size increase we observed after this change for Meta's Android apps. This increases the size of compiler-rt by 1.6 KB, which is small by itself, but then compiler-rt is statically linked into each SO,

[PATCH] D138975: [perf-training] Support additional test suffixes

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138975/new/ https://reviews.llvm.org/D138975

[PATCH] D138974: [CMake] Support injecting extra dependencies for perf-training

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. Might be worth a mention in either https://llvm.org/docs/AdvancedBuilds.html#multi-stage-pgo, to make it a bit more discoverable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D138157: Make -fsanitize=scudo use scudo_standalone. Delete check-scudo.

smeenai added a comment. We're seeing a CMake error after this: CMake Error at /data/users/smeenai/Source/tcdev/external/llvm-project/compiler-rt/cmake/Modules/AddCompilerRT.cmake:368 (add_library): Error evaluating generator expression: $ Objects of target

[PATCH] D136664: [CMake] Support building crt with the bootstrapping build

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. In D136664#3884796 , @phosek wrote: > In D136664#3882989 , @smeenai wrote: > >> I might be missing it, but

[PATCH] D136664: [CMake] Support building crt with the bootstrapping build

smeenai added a comment. I might be missing it, but I don't see `crt` depending on `builtins` (or vice versa). If there is actually no dep, could we build them together in a single configure, instead of needing to add another one for crt? If there is a dep and I missed it, then this LGTM.

[PATCH] D131963: [libc++] Add custom clang-tidy checks

smeenai added a comment. In D131963#3845092 , @philnik wrote: > In D131963#3831786 , @smeenai wrote: > >> In D131963#3811811 , @ldionne >> wrote: >> >>> I'd be fine with

[PATCH] D131963: [libc++] Add custom clang-tidy checks

smeenai added a comment. In D131963#3811811 , @ldionne wrote: > I'd be fine with this as-is if it works in the CI. > > IIUC, the current blocker for this is that the `ClangConfig.cmake` installed > by LLVM is not robust to the dev packages missing. If

[PATCH] D131153: AArch64: disable asynchronous unwind by default for MachO.

smeenai added a comment. I just discovered this and was about to post on Discourse about it. Glad you're already on it :) I don't think this is quite correct though? It'll turn off unwind tables for AArch64 entirely, whereas we want to keep sync unwind tables. If we want to minimize changes

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

smeenai added a comment. Yeah, this makes sense to me if we factor out the commonalities. I agree that it's nicer to have a single option vs. having to specify every single native tool individually. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

smeenai added a comment. This is breaking our build setup. It causes e.g. the Clang resource headers to be installed to `lib64/clang` instead of `lib/clang`. Given this and the other breakages, can we revert for now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

smeenai added a comment. I confirmed that this fixes all remaining issues on our end. Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131874/new/ https://reviews.llvm.org/D131874 ___ cfe-commits mailing list

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

smeenai added a comment. In D131528#3723231 , @ayermolo wrote: > In D131528#3722395 , @shafik wrote: > >> In D131528#3719430 , @ayermolo >> wrote: >> >>> Was this and

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. In D131307#3726643 , @erichkeane wrote: > In D131307#3726631 , @smeenai wrote: > >> Was it intended that the warning generated here isn't silenced by `-w`, only >> by an explicit

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. Was it intended that the warning generated here isn't silenced by `-w`, only by an explicit `-Wno-enum-constexpr-conversion` (or `-Wno-everythning`), and that `-Wno-error` doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG for an example. Repository:

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

smeenai added a comment. Should there be a test case to verify that the warning isn't triggered in a non-constexpr context anymore? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131528/new/ https://reviews.llvm.org/D131528 ___ cfe-commits

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. In D130058#3708209 , @ayermolo wrote: > +2 turning it in to warning. It's breaking our builds, due to boost. :( This was done by D131307 , which I'm hoping will land soon :) CHANGES SINCE

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. All right, SGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131307/new/ https://reviews.llvm.org/D131307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. In D131307#3704709 , @thakis wrote: > It's already an error, but it's a warning default-mapped to an error. You can > -Wno-error=name to downgrade it into a warning, but that requires an explicit > action. So people are

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. Thank you! I'm worried that users might miss this if it's only in the release notes, and then we'd be in a similar situation again when we try converting it to an error. Maybe you could also include the bit about the diagnostic turning into error-only in the

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. In D130058#3698213 , @shafik wrote: >> Given that we have a non-obvious (at least to me) issue in a widely used >> third-party library, would we consider giving users some way to opt out of >> this error, at least as a

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. https://godbolt.org/z/axhbj3MPE is a simpler repro in Godbolt with Boost 1.79 (the latest version). I agree that it'll be tricky to manage expectations if we downgrade to a warning, but I also suspect that the first time many people will run into this new error is

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

smeenai added a comment. Sorry for chiming in a bit late here. I'm working on getting a large internal codebase to compile after this change. Most of the issues are easy enough to deal with, but there's one Boost error I'd like to highlight: P8289 . (We might be

  1   2   3   4   5   6   7   8   >