[PATCH] D96278: [clang][cli] Extract FileSystem and Migrator options parsing/generation

dmajor added a comment. Thank you for the quick followup! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96278/new/ https://reviews.llvm.org/D96278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96278: [clang][cli] Extract FileSystem and Migrator options parsing/generation

dmajor added a comment. This broke the windows bot: http://lab.llvm.org:8011/#/builders/86/builds/7540/steps/6/logs/stdio FAILED: tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/CompilerInvocation.cpp.obj

[PATCH] D95166: Disable rosegment for old Android versions.

dmajor added a comment. It does feel kind of awkward to me that _this_ is the patch that ends up breaking the builds, versus something at the cmake level that says "you are explicitly unsupported". Also it's unfortunate that this landed just hours before 12.0.0 branched. Had it landed

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

dmajor added a comment. If I'm reading git correctly, the change is still present on the 12.x branch. Should it be reverted there too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91913/new/ https://reviews.llvm.org/D91913

[PATCH] D95166: Disable rosegment for old Android versions.

dmajor added a comment. Firefox has a build break from this change. In certain Android configurations we use bfd or gold. The statement in the commit message "Android only supports LLD" is news to me, could you point me to any references for this? Repository: rG LLVM Github Monorepo

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

dmajor added a comment. Our bots are green at b43c26d036dc . Many thanks @hvdijk for the quick response. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91913/new/

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

dmajor added a comment. On closer inspection (sorry, I'm juggling too many things this morning) it seems gcc does give a matching `int nArgs = 1` under `-std=c++17` mode. So potentially the user code is wrong? One thing that puzzles me is why this only broke on our Windows bots. I am not clear

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

dmajor added a comment. The bot that reported this failure was using: `clang -cc1 -triple x86_64-pc-windows-msvc19.16.27038 -fms-extensions -fms-compatibility -fms-compatibility-version=19.16.27038 -std=c++17` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

dmajor added a comment. We have a downstream build break due to this commit. One of our files has some convoluted arg-counting logic that now returns a different count, which does not match gcc: https://godbolt.org/z/W8caMr (Note: At time of writing, the clang trunk on godbolt doesn't yet have

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

dmajor added a comment. I tried the updated patch against our build and it was successful. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92936/new/ https://reviews.llvm.org/D92936 ___

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

dmajor added a comment. Before the revert, our bots hit the following issue where we only error out when `-Wall` is given, so there's definitely something strange going on. Also happens with @Quuxplusone's suggested change applied. $ cat test.cpp template < class > class RefPtr { public:

[PATCH] D91813: [PGO] verify BFI counts after loading profile data

dmajor added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:1948 + // xur + fprintf(stderr, "hihi 2\n"); + // FuncAttrs.addAttribute(llvm::Attribute::Hot); This looks like something for temporary local debugging, was it intentional

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

dmajor added a comment. To connect the dots for posterity: the followup was landed in D92349 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91747/new/ https://reviews.llvm.org/D91747

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

dmajor added a comment. In D91747#2412346 , @alanphipps wrote: > Looks like _LIBCPP_HAS_NO_THREADS is being set for libcxxabi, and the build > now fails with this change: > > llvm-project/libcxxabi/../libcxx/include/__config:1172:2: error: >

[PATCH] D91330: [clangd] Ensure we test for compatibility of serialized index format

dmajor added a comment. In D91330#2394415 , @kadircet wrote: > Looks like it is already green at > http://lab.llvm.org:8011/#/builders/109/builds/2693 Yep, I'm seeing the same, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91330: [clangd] Ensure we test for compatibility of serialized index format

dmajor added a comment. This broke the bots: http://lab.llvm.org:8011/#/builders/109/builds/2682 File "/b/1/clang-x86_64-debian-fast/llvm.obj/tools/clang/tools/extra/clangd/test/lit.site.cfg.py", line 35 config.have_zlib = ^ SyntaxError: invalid syntax

[PATCH] D88609: Use uint64_t for branch weights instead of uint32_t

dmajor added a comment. After 10f2a0d662d8 our self-host PGO builds are hitting: llvm::BranchProbability::getBranchProbability(uint64_t, uint64_t): Assertion `Numerator <= Denominator && "Probability cannot be bigger than

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend

dmajor added a comment. After this commit our bots have a failure on the wasm target, could you please take a look? test.c: double a, b; c() { #pragma STDC FENV_ACCESS ON b == a; } Run `clang -cc1 -triple wasm32-unknown-wasi -emit-obj test.c` (clang needs to be build with

[PATCH] D88005: [clang] [MinGW] Add an implicit .exe suffix even when crosscompiling

dmajor added a comment. This change broke the configure step of Firefox mingw builds. The build bot won't give me the full details, so I'll need to set up a local repro next week if needed, but my hunch is that we have some test like `$CC $CFLAGS conftest.c -o conftest` and then check for the

[PATCH] D83004: [UpdateCCTestChecks] Include generated functions if asked

dmajor added a comment. The expensive-checks bots have been red for several days. Could you please take a look or revert? riscv_generated_funcs.test: *** Bad machine code: MBB exits via unconditional fall-through but ends with a barrier instruction! *** - function:check_boundaries -

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. I threw as many tests as I could find at 9d172c8e9c84 , and I don't see any regressions compared to its parent revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87917: [Sema] Handle objc_super special lookup when checking builtin compatibility

dmajor added a comment. Thanks, this fixes the specific translation unit I was looking at today. It's going to take some time before I have full build results but I don't expect any problems. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

dmajor added a comment. This commit broke Firefox builds on Mac with an error in the SDK headers. Could you please revert if a fix is not readily available? Reproducer: struct objc_super {}; extern "C" id objc_msgSendSuper(struct objc_super *super, SEL op, ...); extern "C" void

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. Thanks for the heads up. I'll schedule a more intensive test run over the weekend while there's less demand for machines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/ https://reviews.llvm.org/D87163

[PATCH] D87701: Do not apply calling conventions to MSVC entry points

dmajor added a comment. This broke Firefox builds too, in one of our helper binaries that uses a `wWinMain`, although I'm having trouble writing a minimal reproducer for it. Simply making a barebones `wWinMain` program doesn't hit the error. If the patch re-lands, please cc me and I'll

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. In D87163#2275896 , @asbirlea wrote: > I checked in a fix in https://reviews.llvm.org/rGfc8200633122, but I haven't > yet verified it addresses all the failures reported. Thanks, I've confirmed that this fixes our tests in their

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. Reduced a bit more: https://godbolt.org/z/j59evK (C++) and https://godbolt.org/z/8xG688 (IR) -- the store at line 43 of `while.end` has been removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87163/new/

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. Here is a reduced-ish repro, I can keep poking at it but maybe this is sufficient for you to notice something: https://godbolt.org/z/endf6d. Center pane is old DSE, right pane is new DSE. I highlighted the problem area with a nop at line 56 of the source, and line 59 of

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. In D87163#2272105 , @fhahn wrote: > I also pushed a fix for a MemorySSA issue that was exposed through DSE + > MemorySSA in c4f1b3144184 > . This > might also

[PATCH] D87163: [DSE] Switch to MemorySSA-backed DSE by default.

dmajor added a comment. In D87163#2270871 , @fhahn wrote: > In D87163#2270444 , @thakis wrote: > >> Heads-up: We're seeing fairly widespread test failures with this in Chromium >> that go away when we force this

[PATCH] D85545: [Diagnostics] Diagnose missing comma in string array initialization

dmajor added a comment. In the Firefox repo this warning is firing on a number of strings that were broken up by clang-format (or humans) for line length, for example

[PATCH] D82562: Implement AVX ABI Warning/error

dmajor added a comment. @erichkeane, could you help me understand what is the action item of these warnings? In Firefox we don't require AVX so our compilations generally don't enable the feature. (A very small number of files do come with AVX versions, mostly in imported media codecs, but

[PATCH] D83494: [libFuzzer] Link libFuzzer's own interceptors when other compiler runtimes are not linked.

dmajor added a comment. After this commit, several of our builds are failing with `FuzzerInterceptors.cpp:30:10: fatal error: 'sanitizer/common_interface_defs.h' file not found`. This is odd because the file certainly seems like it exists. Is there a missing include path somewhere, perhaps?

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

dmajor added a comment. In D65543#2123200 , @dmajor wrote: > In that case, more specifically, the problem I'm seeing is that these flags > are not making it through to a small handful of files: LLVMHello.dll, > PrintFunctionNames.dll,

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

dmajor added a comment. In D65543#2122942 , @dmajor wrote: > After this change, PGO builds of clang on Windows can no longer find > `clang_rt.profile-x86_64.lib`. Is there some cmake magic that could > potentially make this seamless? Or is it expected

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

dmajor added a comment. After this change, PGO builds of clang on Windows can no longer find `clang_rt.profile-x86_64.lib`. Is there some cmake magic that could potentially make this seamless? Or is it expected that the burden is on the user to set up the paths manually during the

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dmajor added a comment. Thanks! I'm still working on getting commit access, would one of you be able to submit for me? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74784/new/ https://reviews.llvm.org/D74784 ___ cfe-commits mailing list

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dmajor updated this revision to Diff 246771. dmajor edited the summary of this revision. dmajor added a comment. Updated the tests: three cases for {default, old, new} linkers in the platform-version tests; left alone the tests not specifically targeting this path. CHANGES SINCE LAST ACTION

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dmajor added a comment. @steven_wu, ping, could you clarify about the tests please? >> You should definitely add test for this change. The fact that you change all >> `-mlinker-version=400` to `-mlinker-version=0` but not change any CHECK >> lines means the change is definitely not tested :) >

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dmajor added a comment. In D74784#1882974 , @steven_wu wrote: > I forgot if there is reason to use the option by default at all time (I did > ask that in the previous review but Alex might have given more context > offline). I would really like to

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dmajor created this revision. dmajor added reviewers: arphaman, steven_wu, dexonsmith. dmajor added projects: clang, LLVM. Herald added a subscriber: cfe-commits. (Note, I don't currently have commit access.) The code in llvmorg-10-init-12188-g25ce33a6e4f is a breaking change for users of older

[PATCH] D71579: [driver][darwin] Pass -platform_version flag to the linker instead of the -_version_min flag

dmajor added inline comments. Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:339 // Add the deployment target. - MachOTC.addMinVersionArgs(Args, CmdArgs); + if (!Version[0] || Version[0] >= 520) +MachOTC.addPlatformVersionArgs(Args, CmdArgs);

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

dmajor added a comment. I noticed that this was merged to the 10.0 release branch. Should the merge be reverted while the dust settles on the trunk implementation? FWIW this change breaks code in skia, as used in both Firefox and Chromium. I see that crbug.com/1042470 added the flag as a

[PATCH] D72167: Add support for __declspec(guard(nocf))

dmajor added a comment. I've confirmed that the current patch fixes our CFG failures. Thanks again! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72167/new/ https://reviews.llvm.org/D72167 ___

[PATCH] D72167: Add support for __declspec(guard(nocf))

dmajor added a comment. Is the current patch an interdiff? It would be helpful to have the diff against the master repo; Phabricator can take care of showing interdiffs if necessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72167/new/

[PATCH] D72167: Add support for __declspec(guard(nocf))

dmajor added a comment. Thanks for doing this! When I applied this patch and added `__declspec(guard(nocf))` to the function that was giving us trouble , I still

[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

dmajor added a comment. In D65761#1772031 , @rnk wrote: > I think `-fcf-protection` and `__attribute__((nocf_check))` have to do with > CET and Intel's endbranch instruction or what have you. Similar goals, > different implementation. I think at this

[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

dmajor added a comment. Are there any plans to implement `__declspec(guard(nocf))` or an equivalent mechanism? `__attribute__((nocf_check))` doesn't do anything without the -fcf-protection flag. (https://bugs.llvm.org/show_bug.cgi?id=44096) Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D68351: [profile] Add a mode to continuously sync counter updates to a file

dmajor added inline comments. Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:377 +#if defined(__Fuchsia__) || defined(_WIN32) + PROF_ERR("%s\n", "Continuous mode not yet supported on Fuchsia or Windows."); +#else // defined(__Fuchsia__) || defined(_WIN32)

[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

dmajor added a comment. Hi, I filed https://bugs.llvm.org/show_bug.cgi?id=44049 for some strange crashes that we're seeing because the CFG code overwrites the lower byte of function pointers before jumping to them. (Commenting separately here because I was unable to CC @ajpaverd in Bugzilla)