[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:9552 +// overwrite linkage of explicit template instantiation +// definition/declaration. +return GVA_DiscardableODR; takuto.ikuta wrote: > takuto.ikuta wrote: > > hans wrote: > > >

[PATCH] D53870: [clang-cl] Put dllexport attrs on static locals also in template instantiations (PR39496)

hans added a comment. Bah, I should have known something was going to break if I touched this :-) http://lab.llvm.org:8011/builders/sanitizer-windows/builds/37132 http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/1017 Here's a repro: template struct __declspec(dllimport) S {

[PATCH] D53870: [clang-cl] Put dllexport attrs on static locals also in template instantiations (PR39496)

hans added a comment. r345709 should do it. Repository: rC Clang https://reviews.llvm.org/D53870 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53457: clang-cl: Add "/clang:" pass-through arg support.

hans added a comment. In https://reviews.llvm.org/D53457#1279191, @neerajksingh wrote: > In https://reviews.llvm.org/D53457#1278579, @hans wrote: > > > The `-Xclang` option has the same issue, and I think `/clang:` should work > > similarly, i.e. `/clang:-MF /clang:`. It's not pretty, but at >

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:9552 +// overwrite linkage of explicit template instantiation +// definition/declaration. +return GVA_DiscardableODR; takuto.ikuta wrote: > hans wrote: > > takuto.ikuta wrote: > > >

[PATCH] D56488: clang-cl: Align help texts for /O1 and O2

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm, thanks CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56488/new/ https://reviews.llvm.org/D56488 ___ cfe-commits mailing list cfe-commi

[PATCH] D56489: clang-cl: Fix help text for /O: '/O2y-' means '/O2 /Oy-', not '/O2 /y-'

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. thanks CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56489/new/ https://reviews.llvm.org/D56489 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D56006: [AST] Fix a -Wimplicit-fallthrough warning in ScanfFormatString.cpp

hans added a comment. Sorry for the late reply; I was on vacation. It seems Erich Keane already fixed this in r350941. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56006/new/ https://reviews.llvm.org/D56006 ___ cfe

[PATCH] D56445: [Sema] Teach Clang that aligned allocation is not supported with macosx10.13

hans added a comment. This (I think) caused a bunch of libc++ tests to start failing during testing of the newly created 8.0 branch on my machine, see https://bugs.llvm.org/show_bug.cgi?id=40354 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56445/new/ https://re

[PATCH] D57004: [docs] Add release notes for notable things I've contributed since last release

hans accepted this revision. hans added inline comments. This revision is now accepted and ready to land. Comment at: docs/ReleaseNotes.rst:190 +- For MinGW, clang now produces vtables and RTTI for dllexported classes + without key functions. + mstorsjo wrote: >

[PATCH] D56946: [Documentation] Use HTTPS whenever possible in Clang

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Seems like a good change to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56946/new/ https://reviews.llvm.org/D56946 ___

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

hans added a comment. In D57150#1369423 , @sammccall wrote: > Plan for now is to revert the original change, live with the things that are > broken, cherrypick the revert to the llvm8 branch, then revisit. Sgtm for 8.0. I see the revert landed in r35207

[PATCH] D57268: [dllimport] Don't use RAV to check inlinability

hans added a comment. Yes, it's a little bit unfortunate that the code needs to get more complicated due to this. The RecursiveStmtVisitor idea that you mention sounds like it might be a good idea (but if it turns out not to be, I'm not entirely opposed to this patch either). CHANGES SINCE LA

[PATCH] D57458: [docs][mips] Clang 8.0 Release notes

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks good from my perspective. Let me know if you'd like me to commit, otherwise go and commit directly to the branch when you're ready. Repository: rC Clang CHANGES SINCE LAST ACTION https

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added a comment. Thank you Takuto! I think the functionality looks great now: it's not too complicated :-) I only have comments about the test. Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:3 +// RUN: -fno-dllexport-inlines -emit-llvm -O0 -o -

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added inline comments. Comment at: clang/include/clang/Driver/CLCompatOptions.td:336 HelpText<"Disable precompiled headers, overrides /Yc and /Yu">; +def _SLASH_Zc_dllexportInlines : CLFlag<"Zc:dllexportInlines">; +def _SLASH_Zc_dllexportInlines_ : CLFlag<"Zc:dllexportInl

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added a comment. Thanks! I don't think the new isThisDeclarationADefinition() and isImplicitlyInstantiable() checks are right. Besides that, I only have a few more comments about the test. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5715 + (MD->isThisDeclarationA

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Assuming the test still passes when you put the EXPORTINLINE filecheck prefix back, looks good to me! Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:9 +//

[PATCH] D53457: clang-cl: Add "/clang:" pass-through arg support.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Okay, looks good to me (with a small nit). Comment at: docs/UsersManual.rst:2852 /Brepro Emit an object file which can be reproduced over time + /cla

[PATCH] D54171: [MS] Zero out ECX in __cpuid in intrin.h

hans added a comment. Thanks for fixing! Repository: rL LLVM https://reviews.llvm.org/D54171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53457: clang-cl: Add "/clang:" pass-through arg support.

hans added a comment. In https://reviews.llvm.org/D53457#1291020, @neerajksingh wrote: > Reid, Hans, or someone else with commit access. If the revision looks good, > could you please submit to SVN? > > Any particular testing I should run beforehand? I ran the clang tests locally > on Windows.

[PATCH] D53457: clang-cl: Add "/clang:" pass-through arg support.

This revision was automatically updated to reflect the committed changes. Closed by commit rC346393: clang-cl: Add "/clang:" pass-through arg support. (authored by hans, committed by ). Repository: rC Clang https://reviews.llvm.org/D53457 Files: docs/UsersManual.rst include/clang/Driver/C

[PATCH] D54298: [clang-cl] Add warning for /Zc:dllexportInlines- when the flag is used with /fallback

hans added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5507 CmdArgs.push_back("-fno-dllexport-inlines"); +if (Args.hasArg(options::OPT__SLASH_fallback)) { + const Arg *dllexportInlines = I think the check should be with the co

[PATCH] D54298: [clang-cl] Add warning for /Zc:dllexportInlines- when the flag is used with /fallback

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rL LLVM https://reviews.llvm.org/D54298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D54319: clang-cl: Add documentation for /Zc:dllexportInlines-

hans created this revision. hans added reviewers: takuto.ikuta, thakis, rnk. A great feature needs great documentation. Please take a look! https://reviews.llvm.org/D54319 Files: docs/UsersManual.rst include/clang/Driver/CLCompatOptions.td Index: include/clang/Driver/CLCompatOptions.td ===

[PATCH] D54319: clang-cl: Add documentation for /Zc:dllexportInlines-

This revision was automatically updated to reflect the committed changes. Closed by commit rL346639: clang-cl: Add documentation for /Zc:dllexportInlines- (authored by hans, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D54319?vs=1733

[PATCH] D54426: [clang-cl] Do not allow using both /Zc:dllexportInlines- and /fallback flag

hans added inline comments. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:164 -def warn_drv_non_fallback_argument_clang_cl : Warning< - "option '%0' is ignored when /fallback happens">, - InGroup; +def err_drv_both_fallback_and_dllexport_inlines_cannot_be_use

[PATCH] D54426: [clang-cl] Do not allow using both /Zc:dllexportInlines- and /fallback flag

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rL LLVM https://reviews.llvm.org/D54426 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D54319: clang-cl: Add documentation for /Zc:dllexportInlines-

hans marked 3 inline comments as done. hans added a comment. Thanks for the review! I made adjustments in r346748. Comment at: cfe/trunk/docs/UsersManual.rst:3104 + +This causes the class-level `dllexport` and `dllimport` attributes not to be +applied to inline member functions

[PATCH] D55371: Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

hans edited subscribers, added: cfe-commits; removed: llvm-commits. hans added a comment. -llvm-commits +cfe-commits CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55371/new/ https://reviews.llvm.org/D55371 ___ cfe-commits mailing list cfe-c

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55377/new/ https://reviews.llvm.org/D55377 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D55371: Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

This revision was automatically updated to reflect the committed changes. Closed by commit rL348569: Fix thunks returning memptrs via sret by emitting also scalar return values… (authored by hans, committed by ). Changed prior to commit: https://reviews.llvm.org/D55371?vs=176971&id=177135#toc

[PATCH] D58504: [OpenCL][8.0.0 Release] Notes for OpenCL

hans added a comment. Many thanks! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58504/new/ https://reviews.llvm.org/D58504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailma

[PATCH] D58721: Added release notes for clangd 8

This revision was automatically updated to reflect the committed changes. Closed by commit rL355004: Added release notes for clangd 8 (authored by hans, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D5872

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

hans created this revision. hans added reviewers: efriedma, rnk, rsmith, void. Herald added subscribers: jdoerfert, eraman. Apparently GCC allows this, and there's code relying on it (see bug, which is a release blocker for llvm 8). The idea is to allow expression that would have been allowed if

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

hans marked 6 inline comments as done. hans added a comment. In D58821#1416212 , @joerg wrote: > Can you include a patch for something like (int *)0xdeadbeef on amd64? > That's a valid value for "n", but clearly too large for int. Thanks for > lookin

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

hans updated this revision to Diff 189129. hans marked 2 inline comments as done. hans added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58821/new/ https://reviews.llvm.org/D58821 Files: clang/lib/CodeGen/CGStmt.cpp clang/lib/Sema/SemaStmtAsm.cpp cl

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

hans added a comment. In D58821#1416212 , @joerg wrote: > Can you include a patch for something like (int *)0xdeadbeef on amd64? > That's a valid value for "n", but clearly too large for int. Thanks for > looking at this, it is one of the two large r

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

hans marked 2 inline comments as done. hans added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:1852 +IntResult = +llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity()); + else efriedma wrote: > hans wrote: > > efrie

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

hans updated this revision to Diff 189319. hans added a comment. Extract code to a new method in APValue. Eli, what do you think about something like this? Suggestions for better name welcome :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58821/new/ https://reviews.llvm.org/D58821

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

This revision was automatically updated to reflect the committed changes. Closed by commit rC355491: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890) (authored by hans, committed by ). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.o

[PATCH] D59038: [8.0 Regression] Fix handling of `__builtin_constant_p` inside the enable_if attribute.

hans added a comment. I think I'll have to defer to Richard for reviewing this, since I'm not familiar with the code. Since it's a regression we probably want to merge it to clang 8. Can you give any guidance to how bad a regression this is, e.g. does it break some important piece of code? Re

[PATCH] D59038: [8.0 Regression] Fix handling of `__builtin_constant_p` inside the enable_if attribute.

hans added a comment. In D59038#1422890 , @rsmith wrote: > LGTM, and I think this is safe enough to take for Clang 8. Do you think the severity is high enough to spin another release candidate? My concern is that since this didn't show up in testing unt

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

hans added inline comments. Comment at: test/Frontend/absolute-paths-windows.test:4 +// RUN: mkdir -p %t.dir\real +// RUN: cmd /c mklink /j %t.dir\junc %t.dir\real +// RUN: echo "wrong code" > %t.dir\real\foo.cpp This requires (a recent version of) Win 10 or late

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

hans added a comment. I don't feel like I have enough background on what you're trying to do to review this. It sounds related to clangd issues maybe? "If the source file path contains directory junctions, and we resolve them when printing diagnostic messages" I didn't think Clang tried to res

[PATCH] D60728: [clang] [test] Add a (xfailing) test for PR41027

hans added a comment. What's the value in checking in this xfail'ed test without an actual fix for the problem? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60728/new/ https://reviews.llvm.org/D60728 ___ cfe-commits mailing list cfe-commi

[PATCH] D60728: [clang] [test] Add a (xfailing) test for PR41027

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. In D60728#1468713 , @krytarowski wrote: > In D60728#1468486 , @hans wrote: > > > What's the value in checking in th

[PATCH] D60800: [MS] Emit S_HEAPALLOCSITE debug info

hans added a comment. I don't really know about the functionality here, just adding a few comments on the code itself. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1966 + QualType PointeeTy = D.getTypePtr()->getPointeeType(); + llvm::DIType *DI = getOrCreateType(PointeeTy,

[PATCH] D60627: [MSVC] Use the correct casing of HostX64/HostX86

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60627/new/ https://reviews.llvm.org/D60627 ___ cfe-c

[PATCH] D60997: Fix unquoted spaces in args in clang --verbose output

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Thanks for fixing! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60997/new/ https://reviews.llvm.org/D60997 ___ cfe-

[PATCH] D61029: clang-cl: List valid values for /std: in /? output

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61029/new/ https://reviews.llvm.org/D61029 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D60997: Fix unquoted spaces in args in clang --verbose output

This revision was automatically updated to reflect the committed changes. Closed by commit rC359077: Fix unquoted spaces in args in clang --verbose output (authored by hans, committed by ). Changed prior to commit: https://reviews.llvm.org/D60997?vs=196191&id=196398#toc Repository: rC Clang

[PATCH] D61118: [MinGW] Fix dllexport of explicit template instantiation

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61118/new/ https://reviews.llvm.org/D61118 ___ cfe-c

[PATCH] D61175: [MinGW] Don't let template instantiation declarations cover nested classes

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Seems okay to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61175/new/ https://reviews.llvm.org/D61175 ___ cfe-c

[PATCH] D61176: [MinGW] Do dllexport inline methods in template instantiation

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61176/new/ https://reviews.llvm.org/D61176 ___ cfe-commits mailin

[PATCH] D61177: [MinGW] Always emit local typeinfo

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61177/new/ https://reviews.llvm.org/D61177 ___ cfe-commits mailin

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

hans added a comment. In https://reviews.llvm.org/D52193#1237468, @zturner wrote: > What about the timings of clang-cl without /MP? And one using Ninja rather than msbuild. I think the real question is whether we want clang and clang-cl to do this. I'm not sure we do as it adds complexity for

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

hans added a comment. Thanks for adding the Ninja numbers. It confirms that Ninja is significantly faster than MSBuild + /MP. Since that's the case, maybe we should be poking MS about making MSBuild faster instead of adding /MP support to Clang? Or making it easier to use Ninja in VS projects?

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

hans created this revision. hans added reviewers: thakis, rnk. This provides better help text in "clang-cl /?". Also it cleans things up a bit: previously "/Od" could be handled either as a separate flag aliased to "-O0", or by the main optimization flag processing in TranslateOptArg. With this

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

hans added a comment. Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch. Comment at: test/Driver/Xarch.c:5 +// RUN: not grep ' "-O3" ' %t.log +// RUN: grep "argument unused during compilation: '-Xarch_i386 -O3'" %t.log

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

hans updated this revision to Diff 166252. hans added a comment. Uploading new diff. https://reviews.llvm.org/D52266 Files: include/clang/Driver/CLCompatOptions.td lib/Driver/ToolChains/MSVC.cpp test/Driver/Xarch.c Index: test/Driver/Xarch.c ==

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added a comment. In https://reviews.llvm.org/D51340#1243331, @takuto.ikuta wrote: > Ping? > > This patch reduced obj size largely, and I expect this makes distributed > build (like goma) faster by reducing data transferred on network. I'll try to look at it this week. Have you confirmed

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

hans marked 7 inline comments as done. hans added a comment. > Thoughts on "As far as I can tell we do not make > /https://reviews.llvm.org/owners/package/1/ and > /https://reviews.llvm.org/owners/package/2/ imply /Gs -- we keep it at the > default value of 4096 (?) Fixing this probably means i

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

hans updated this revision to Diff 166862. hans added a comment. Addressing all comments. https://reviews.llvm.org/D52266 Files: include/clang/Driver/CLCompatOptions.td lib/Driver/ToolChains/MSVC.cpp test/Driver/Xarch.c Index: test/Driver/Xarch.c =

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

hans added a comment. In https://reviews.llvm.org/D52266#1244733, @thakis wrote: > > Actually, trying this out with MSVC, I don't see any __chkstk calls with > > /https://reviews.llvm.org/owners/package/1/, or with eg. /Gs1 for that > > matter: > > (.guess the bug should also cover to eventuall

[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants

This revision was automatically updated to reflect the committed changes. Closed by commit rC342977: [clang-cl] Provide separate flags for all the /O variants (authored by hans, committed by ). Changed prior to commit: https://reviews.llvm.org/D52266?vs=166862&id=166884#toc Repository: rC Cl

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

hans created this revision. hans added reviewers: thakis, rnk. Also remove text saying /https://reviews.llvm.org/owners/package/1/ and /https://reviews.llvm.org/owners/package/2/ implies /Gs. They don't seem to do that, at least with modern MSVC (see bug). https://reviews.llvm.org/D52499 File

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

hans added a comment. In https://reviews.llvm.org/D52499#1245474, @majnemer wrote: > FWIW, Microsoft's newest documentation does not say that `/O2` and `/O1` > imply `/Gs`: > https://docs.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed?view=vs-2017 Great, thanks for

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

This revision was automatically updated to reflect the committed changes. Closed by commit rL343077: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074) (authored by hans, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

This revision was automatically updated to reflect the committed changes. Closed by commit rC343077: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074) (authored by hans, committed by ). Repository: rL LLVM https://reviews.llvm.org/D52499 Files: include/clang/Driver/CLCompat

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

hans added inline comments. Comment at: include/clang/Driver/CLCompatOptions.td:94 +def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">, + Alias, AliasArgs<["4096"]>; def _SLASH_Gs : CLJoined<"Gs">, thakis wrote: > https://docs.microsoft.com/en-us/cpp/bu

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added a comment. The static local stuff still makes me nervous. How much does Chromium break if we just ignore the problem? And how does -fvisibility-inlines-hidden handle the issue? Also, Sema already has a check for static locals in C inline functions: $ echo "inline void f() { static

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

hans created this revision. hans added reviewers: rnk, thakis, zturner, steveire. Add a /showFilenames option for users who want clang to echo the currently compiled filename. MSVC does this echoing by default, and it's useful for showing progress in build systems that doesn't otherwise provide

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

hans added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:792 Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name); + if (Args.hasArg(OPT_echo_main_file_name)) +llvm::outs() << Opts.MainFileName << "\n"; zturner wrote: > steveire

[PATCH] D52798: [cl-compat] Change /JMC from unsupported to ignored.

hans added a comment. Thanks for fixing! Please also update the test in test/Driver/cl-options.c and include me on clang-cl code reviews. Repository: rL LLVM https://reviews.llvm.org/D52798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

hans added a comment. In https://reviews.llvm.org/D52773#1252584, @zturner wrote: > Canyou add a test that demonstrates that multiple input files on the command > line will print all of them? Will do. > Also does this really need to be a cc1 arg? Why not just print it in the > driver? Yeah

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

hans added a comment. In https://reviews.llvm.org/D51340#1254898, @takuto.ikuta wrote: > Ping? Sorry, I know I'm being slow to look at this :-( I have not forgotten, it's just a lot of other things happening at the same time. https://reviews.llvm.org/D51340 ___

[PATCH] D57427: [CodeGen][ObjC] Fix assert on calling `__builtin_constant_p` with ObjC objects.

hans added a comment. What's the status of this patch? Is it something we should merge onto the 8.0 release branch? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57427/new/ https://reviews.llvm.org/D57427 ___ cfe-commits mailing list cfe-c

[PATCH] D57427: [CodeGen][ObjC] Fix assert on calling `__builtin_constant_p` with ObjC objects.

hans added a comment. Herald added a subscriber: jdoerfert. > Also looks like in practice it shouldn't be a serious problem, > `__builtin_constant_p` can be optimized out. For example, see > https://godbolt.org/z/e1b4dB Okay, I'm not going to worry about it then. Thanks. Repository: rL LLVM

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

hans removed a subscriber: hansw. hans added a comment. In D57948#1399683 , @riccibruno wrote: > Done, thanks! That was https://bugs.llvm.org/show_bug.cgi?id=40742 and it's now been merged. Repository: rC Clang CHANGES SINCE LAST ACTION https://re

[PATCH] D58377: Remove extraneous space in MSVC-style diagnostic output

hans created this revision. hans added a reviewer: thakis. Herald added a subscriber: jdoerfert. There was an extra space between the file location and the diagnostic message: /tmp/a.c(1,12): warning: unused parameter 'unused' the tests didn't catch this due to FileCheck not running in --stri

[PATCH] D58377: Remove extraneous space in MSVC-style diagnostic output

hans updated this revision to Diff 187320. hans added a comment. Output a char instead of a one-character string. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58377/new/ https://reviews.llvm.org/D58377 Files: clang/lib/Frontend/TextDiagnostic.cpp clang/test/Misc/diag-format.c In

[PATCH] D58377: Remove extraneous space in MSVC-style diagnostic output

This revision was automatically updated to reflect the committed changes. Closed by commit rC354351: Remove extraneous space in MSVC-style diagnostic output (authored by hans, committed by ). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D58377?vs=187320&id=18

[PATCH] D57523: Fix uninitialized value in ABIArgInfo

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Herald added a subscriber: jdoerfert. Herald added a project: clang. I've been staring at this, trying to figure out if the code somehow ends up using the uninitialized values, but I can't find it.

[PATCH] D58504: [OpenCL][8.0.0 Release] Notes for OpenCL

hans added inline comments. Comment at: docs/ReleaseNotes.rst:228 -OpenCL C Language Changes in Clang +OpenCL Language Changes in Clang -- Anastasia wrote: > Anastasia wrote: > > AlexeySotkin wrote: > > > AlexeySotkin wrote: > >

[PATCH] D33852: Enable __declspec(selectany) on linux

hans added a comment. In https://reviews.llvm.org/D33852#854213, @aaron.ballman wrote: > In https://reviews.llvm.org/D33852#854176, @Prazek wrote: > > > In https://reviews.llvm.org/D33852#853982, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D33852#853410, @Prazek wrote: > > > > > >

[PATCH] D26764: [compiler-rt] [cmake] Remove i686 target that is duplicate to i386

hans added inline comments. Comment at: compiler-rt/trunk/lib/asan/scripts/asan_device_setup:98 if [[ $_ABI == x86* ]]; then -_ARCH=i686 +_ARCH=i386 elif [[ $_ABI == armeabi* ]]; then There are lots of copies of this script in various pr

[PATCH] D26764: [compiler-rt] [cmake] Remove i686 target that is duplicate to i386

hans added a comment. In https://reviews.llvm.org/D26764#855791, @eugenis wrote: > IMHO it was a very bad idea to include the "architecture name" (not even a > target triple!) in the library path in the first place. No one else does > that. GCC made the right choice and called theirs "libasan.s

[PATCH] D37278: Restore clang_rt library name on i686-android.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Thanks! Looks good to me. Comment at: clang/lib/Driver/ToolChain.cpp:308 + if (TC.getArch() == llvm::Triple::x86 && Triple.isAndroid()) +return "i686"; Ma

[PATCH] D37336: [clang-cl] Explicitly set object format to COFF in CL mode

hans added a comment. Seems reasonable, but should probably have a test. > This can be observed on the clang's Driver/cl-pch.c test with AArch64 as a > default target. But why isn't that failure showing on some buildbot, then? Repository: rL LLVM https://reviews.llvm.org/D37336

[PATCH] D37336: [clang-cl] Explicitly set object format to COFF in CL mode

hans added a comment. In https://reviews.llvm.org/D37336#857814, @iid_iunknown wrote: > In https://reviews.llvm.org/D37336#857802, @hans wrote: > > > But why isn't that failure showing on some buildbot, then? > > > The test needs `system-windows` to run: > > // REQUIRES: system-windows > > > Th

[PATCH] D37336: [clang-cl] Explicitly set object format to COFF in CL mode

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rL LLVM https://reviews.llvm.org/D37336 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D63387: clang: Promote -fdebug-compilation-dir from cc1 flag to driver-level flag

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/include/clang/Driver/Options.td:717 +def fdebug_compilation_dir : Separate<["-"], "fdebug-compilation-dir">, +Group, Flags<[CC1Option, CC1AsOption, Cor

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. I tried replying on the cfe-commits email, but both Pengfei and Wei's email addresses seem to bounce, so replying here instead: This broke Chromium for 32-bit Linux: https://bugs.chromium.org/p/chromium/issues/detail?id=974542#c5 It's not clear what's happening yet, bet ju

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. In D59744#1547445 , @wxiao3 wrote: > @hans > > Please make sure all Chromium for 32-bit Linux libraries are following System > V ABI (i.e., m64 is passed on mmx register). I suspect that there are some > hand written assembly code i

[PATCH] D63473: Support -fclang-abi-compat=8.0 to keep old ABI behavior

hans added a comment. > The System V i386 bug fix (https://reviews.llvm.org/D59744) makes it > impossible Please refer to the svn revision instead of the code review. > for 32-bit Linux Chromium to write an assembly function that works with both > trunk clang and clang 8.0.0, which makes it di

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. In D59744#1549675 , @wxiao3 wrote: > Can anyone provide me some small reproducers code for the issue tripped over > by Chromium / Skia? Sorry, I don't have a small repro yet. I'm still working on finding out exactly what's happeni

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. In D59744#1549746 , @hans wrote: > In D59744#1549675 , @wxiao3 wrote: > > > Can anyone provide me some small reproducers code for the issue tripped > > over by Chromium / Skia? > > > Sorry, I

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. I've reverted in r363790 until a solution can be found. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. > $ bin/clang -m32 -O0 /tmp/a.c && ./a.out > -nan > > Before your change, it prints 3.14. I looked through the Intel manual to understand what's happening in detail: When we return from f() with the new ABI, we write to the %mm0 register, and as a side effect: (9.5

<    1   2   3   4   5   6   7   8   9   10   >