[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

inglorion added a comment. Thanks for the reply! FWIW, this warning is triggered by code which uses glib, which contains a number of function(some_type*) to function(void*) casts, for example here:

[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

inglorion added a comment. Is this intended to warn on code that casts a function taking a pointer to some non-void type to a function that takes a void*? void set(void (*g)(int*)) { f = (void(*)(void*)) g; } gives me warning: cast from 'void (*)(int *)' to 'void (*)(void *)'

[PATCH] D108603: [clang][codegen] Set CurLinkModule in CodeGenAction::ExecuteAction

This revision was automatically updated to reflect the committed changes. Closed by commit rG1c829ce1e362: [clang][codegen] Set CurLinkModule in CodeGenAction::ExecuteAction (authored by inglorion). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108603: [clang][codegen] Set CurLinkModule in CodeGenAction::ExecuteAction

inglorion created this revision. inglorion requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. CodeGenAction::ExecuteAction creates a BackendConsumer for the purpose of handling diagnostics. The BackendConsumer's DiagnosticHandlerImpl method

[PATCH] D108499: [clang][codegen] Don't assert on CurLinkModule for silenced diagnostic

inglorion added a comment. There are some possible follow-ups here: 1. Making sure CurLinkModule is set in all (or at least more) cases. 2. Addressing the FIXME about warnings and notes. I have some partial implementations of those ideas, but I figure we can take this change first while I

[PATCH] D108499: [clang][codegen] Don't assert on CurLinkModule for silenced diagnostic

inglorion created this revision. inglorion requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fixes PR51564. Under certain circumstances, it is possible to enter Clang's BackendConsumer::DiagnosticHandlerImpl without CurLinkModule set. For

[PATCH] D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=

inglorion added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3758 + // Normally -gsplit-dwarf is only useful with -gN. For -gsplit-dwarf in the + // backend phase of a distributed ThinLTO which does object file generation + // and no IR generation, -gN

[PATCH] D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=

inglorion added a comment. Can we not just make -gsplit-dwarf set the DwarfFissionKind without also looking at other things? That way, we don't have to worry about detecting the right mix of -g, -g2, -fthinlto-index, -x ir, bitcode input, etc. If there is some reason why this doesn't work, I

[PATCH] D80391: [Driver] Don't make -gsplit-dwarf imply -g2

inglorion added a comment. For Chrome on Chrome OS, this is https://crbug.com/1158215 Here, we saw our links fail with "output file too large". Investigation revealed that debug info was being included in the binary, instead of in .dwo files as expected. That turned out to be caused by this

[PATCH] D81203: clang-cl: accept -f[no-]data-sections and -f[no-]function-sections

inglorion abandoned this revision. inglorion added a comment. Ok, thanks for clarifying. The benefit this would get us is not having to spell the flags differently depending on whether the frontend is clang or clang-cl. I figured this would fit in with all the -f options we already take and

[PATCH] D81203: clang-cl: accept -f[no-]data-sections and -f[no-]function-sections

inglorion created this revision. inglorion added reviewers: hans, thakis. Herald added a project: clang. This adds -fdata-sections, -ffunction-sections, and their negations to the list of -f options accepted by clang-cl. As a result, these options can be used with the same spelling for both clang

[PATCH] D64458: add -fthinlto-index= option to clang-cl

This revision was automatically updated to reflect the committed changes. Closed by commit rL366146: add -fthinlto-index= option to clang-cl (authored by inglorion, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D64458: add -fthinlto-index= option to clang-cl

inglorion updated this revision to Diff 209954. inglorion added a comment. Simplified after rebasing on top of r366127. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64458/new/ https://reviews.llvm.org/D64458 Files:

[PATCH] D64610: [clang] allow -fthinlto-index= without -x ir

This revision was automatically updated to reflect the committed changes. inglorion marked an inline comment as done. Closed by commit rL366127: [clang] allow -fthinlto-index= without -x ir (authored by inglorion, committed by ). Herald added a project: LLVM. Herald added a subscriber:

[PATCH] D64610: [clang] allow -fthinlto-index= without -x ir

inglorion updated this revision to Diff 209948. inglorion added a comment. Fix typo pointed out by MaskRay (thanks!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64610/new/ https://reviews.llvm.org/D64610 Files:

[PATCH] D64610: [clang] allow -fthinlto-index= without -x ir

inglorion added a comment. This was suggested on D64458 . If we take this change, D64458 can be simplified to just accepting -fthinlto-index= as a CoreOption without needing any further changes to Driver.cpp. I also changed

[PATCH] D64610: [clang] allow -fthinlto-index= without -x ir

inglorion created this revision. inglorion added reviewers: tejohnson, rnk, pcc. Herald added subscribers: arphaman, dexonsmith, steven_wu, mehdi_amini. Herald added a project: clang. Previously, passing -fthinlto-index= to clang required that bitcode files be explicitly marked by -x ir. This

[PATCH] D64458: add -fthinlto-index= option to clang-cl

inglorion added a comment. > Do we really need to support clang-cl syntax here? Can't the user of > -fthinlto-index= use the regular clang driver? We could, of course, require using the clang driver for this feature. But I would argue that clang-cl exists to make it easy for projects that use

[PATCH] D64458: add -fthinlto-index= option to clang-cl

inglorion added a comment. This is similar to r254927 for the clang driver, and reuses the tests from that commit for the backend part. On the frontend, it differs in that clang-cl does not support -x (per the comment in clang/lib/Driver/Driver.cpp line 2068: "// No driver mode exposes -x and

[PATCH] D64458: add -fthinlto-index= option to clang-cl

inglorion created this revision. inglorion added reviewers: tejohnson, pcc, rnk. Herald added subscribers: arphaman, dexonsmith, steven_wu, mehdi_amini. Herald added a project: clang. This adds a -fthinlto-index= option to clang-cl, which allows it to be used to drive ThinLTO backend passes. This

[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

inglorion accepted this revision. inglorion added a comment. This revision is now accepted and ready to land. Thank you. Given that proceeding with the wrong value will result in either an undefined reference or a reference to what might well be the wrong function at link time, I think making

[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

inglorion added a comment. Can you clarify "which will usually result in a linker error"? E.g. an example of link.exe rejecting the object file or the wrong function being called. The reason I ask is that if we can be sure at compile time that the resulting code will not work or do the wrong

[PATCH] D50877: [MS] Mangle a hash of the main file path into anonymous namespaces

inglorion added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:389 + if (FE) { +llvm::MD5 Hasher; +llvm::MD5::MD5Result Hash; Instead of MD5, can we use xxhash (or whatever the fastest hash algorithm in LLVM is these days)? I don't

[PATCH] D48601: Added -fcrash-diagnostics-dir flag

This revision was automatically updated to reflect the committed changes. Closed by commit rL336604: Added -fcrash-diagnostics-dir flag (authored by inglorion, committed by ). Changed prior to commit: https://reviews.llvm.org/D48601?vs=153773=154695#toc Repository: rL LLVM

[PATCH] D37943: [docs] add Windows examples to ThinLTO.rst

This revision was automatically updated to reflect the committed changes. Closed by commit rL313425: [docs] add Windows examples to ThinLTO.rst (authored by inglorion). Changed prior to commit: https://reviews.llvm.org/D37943?vs=115522=115524#toc Repository: rL LLVM

[PATCH] D37943: [docs] add Windows examples to ThinLTO.rst

inglorion created this revision. Herald added subscribers: eraman, mehdi_amini. https://reviews.llvm.org/D37943 Files: clang/docs/ThinLTO.rst Index: clang/docs/ThinLTO.rst === --- clang/docs/ThinLTO.rst +++

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

This revision was automatically updated to reflect the committed changes. Closed by commit rL312965: [codeview] omit debug locations for nested exprs unless column info enabled (authored by inglorion). Changed prior to commit: https://reviews.llvm.org/D37529?vs=114715=114716#toc Repository:

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114715. inglorion added a comment. renamed get{Nested,}ExpressionLocationsEnabled and moved it into CodeGetModule.cpp https://reviews.llvm.org/D37529 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CodeGenModule.cpp

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114465. inglorion added a comment. Of course, ApplyDebugLocation is also a perfectly legitimate way to add a debug location to nodes that are not nested inside nodes that already have a location. I updated the diff so that we do end up applying the

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114463. inglorion added a comment. added examples suggested by @zturner, verified step over and step into specific behavior matches MSVC, and added tests for them https://reviews.llvm.org/D37529 Files: clang/lib/CodeGen/CGDebugInfo.cpp

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114299. inglorion added a comment. removed compound statement; that was only in there to double check that the debugger stops on the first child statement, but that's true with or without this change https://reviews.llvm.org/D37529 Files:

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion added a comment. @zturner: I am still thinking about your comment about other cases to test. My concern is that there are very many possible combinations. I'm actually not too concerned about not exactly matching cl's behavior in every single case. The difference in behavior here is

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114292. inglorion added a comment. Following conversation with @rnk, I managed to whittle this down to a very small change that seems to do what we need. Step into specific works and single stepping through the program behaves similarly whether compiled

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion planned changes to this revision. inglorion added a comment. rnk and I talked about a different approach. The idea is to explicitly emit locations in some cases (e.g. inside compound statements, the braces of for loops, ...), and otherwise emit locations only when emitting column info

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114097. inglorion marked 5 inline comments as done. inglorion added a comment. I limited the change to only calls, returns, and declarations. I also updated the test case to include a multi-variable declaration, a while loop, a for loop, and an if statement

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-nested-exprs.cpp:44 + int a = bar(x, y) + + baz(x, z) + + qux(y, z); zturner wrote: > inglorion wrote: > > zturner wrote: > > > Can you make a function called `int

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion created this revision. Herald added a subscriber: aprantl. Microsoft Visual Studio expects debug locations to correspond to statements. We used to emit locations for expressions nested inside statements. This would confuse the debugger, causing it to stop multiple times on the same line

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:45 + } + return IDL; +} inglorion wrote: > rnk wrote: > > Does MSVC accept this? I think it will emit the copy ctor call in an -O0 > > build. > I wrote this thinking that the right

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:45 + } + return IDL; +} rnk wrote: > Does MSVC accept this? I think it will emit the copy ctor call in an -O0 > build. I wrote this thinking that the right thing would happen under copy

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

inglorion updated this revision to Diff 114060. inglorion added a comment. removed accidentally left in include and reformatted mangled comment https://reviews.llvm.org/D37529 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/lib/CodeGen/CGStmt.cpp

[PATCH] D31633: test for thinlto handling of internal linkage

inglorion abandoned this revision. inglorion added a comment. We will not be needing this. https://reviews.llvm.org/D31633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31633: test for thinlto handling of internal linkage

inglorion added a comment. This is the test for https://reviews.llvm.org/D31632. https://reviews.llvm.org/D31633 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31633: test for thinlto handling of internal linkage

inglorion created this revision. Herald added a subscriber: Prazek. https://reviews.llvm.org/D31633 Files: test/CodeGenCXX/thinlto-promote-internals.cpp Index: test/CodeGenCXX/thinlto-promote-internals.cpp === --- /dev/null +++

[PATCH] D30663: Use filename in linemarker when compiling preprocessed source (Revised)

inglorion accepted this revision. inglorion added a comment. This revision is now accepted and ready to land. Fixing the other issues in a follow-up seems fine. This lgtm. https://reviews.llvm.org/D30663 ___ cfe-commits mailing list

[PATCH] D30663: Use filename in linemarker when compiling preprocessed source (Revised)

inglorion added inline comments. Comment at: test/Frontend/preprocessed-input.c:3 +// RUN: %clang -emit-llvm -S -o - %t.i | FileCheck %s +// CHECK: source_filename = {{.*}}preprocessed-input.c"{{$}} Actually, I think you don't even have to run the preprocessor -

[PATCH] D30591: Introduce the feature "linux" for tests only for linux

inglorion added a comment. Checking for linux when really you want to check for ELF doesn't seem right. In this case, I think there is an better way to do it; instead of relying on llvm-objdump, could you emit an LLVM assembly file and check that for presence of the string you want? I think if

[PATCH] D30239: enable -flto=thin in clang-cl

This revision was automatically updated to reflect the committed changes. Closed by commit rL296373: enable -flto=thin in clang-cl (authored by inglorion). Changed prior to commit: https://reviews.llvm.org/D30239?vs=89598=89913#toc Repository: rL LLVM https://reviews.llvm.org/D30239

[PATCH] D30239: enable -flto=thin in clang-cl

inglorion updated this revision to Diff 89598. inglorion added a comment. changed error message https://reviews.llvm.org/D30239 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td lib/Driver/Driver.cpp test/Driver/cl-options.c Index:

[PATCH] D30239: enable -flto=thin in clang-cl

inglorion updated this revision to Diff 89590. inglorion added a comment. fail early with a friendlier message when using -flto without -fuse-ld=lld https://reviews.llvm.org/D30239 Files: include/clang/Driver/Options.td lib/Driver/Driver.cpp test/Driver/cl-options.c Index:

[PATCH] D30239: enable -flto=thin in clang-cl

inglorion updated this revision to Diff 89583. inglorion added a comment. added missing -- https://reviews.llvm.org/D30239 Files: include/clang/Driver/Options.td test/Driver/cl-options.c Index: test/Driver/cl-options.c ===

[PATCH] D30239: enable -flto=thin in clang-cl

inglorion added inline comments. Comment at: test/Driver/cl-options.c:525 +// RUN: %clang_cl -### /c -flto %s 2>&1 | FileCheck -check-prefix=LTO %s +// LTO: -flto hans wrote: > This needs `--` before `%s`, otherwise if `%s` expands to e.g. `/Users/foo` > it

[PATCH] D30239: enable -flto=thin in clang-cl

inglorion updated this revision to Diff 89577. inglorion retitled this revision from "enable -flto=thin, -flto-jobs=, and -fthinlto-index= in clang-cl" to "enable -flto=thin in clang-cl". inglorion added a comment. Implemented @hans's suggestion of moving the tests into cl-options.c. Also

[PATCH] D30239: enable -flto=thin, -flto-jobs=, and -fthinlto-index= in clang-cl

inglorion added a comment. @mehdi_amini: > Is clang-cl using lld as default? How is the switch done? Ideally we should > have a nice error message from the driver if -flto is used without lld. I believe we use link.exe by default. You can use lld by passing -fuse-ld=lld to the compiler. I

[PATCH] D25216: Improve error message when referencing a non-tag type with a tag

inglorion added a comment. I like it. I'll give others a little time to respond, but if no objections are raised and nobody else accepts first, I'll accept it tomorrow. https://reviews.llvm.org/D25216 ___ cfe-commits mailing list