[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. In D74846#1888410 , @llunak wrote: > Upon further investigation it turns out I probably should not have enabled > those two places for PCH in D69778 at all, > since they do not deal with -fmodul

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. In D74846#1891486 , @llunak wrote: > In D74846#1889826 , @dblaikie wrote: > > > I know it's a bit of an awkward situation to test - but please include one > > to demonstrate why the origin

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:61 HANDLE_DI_FLAG((1 << 29), AllCallsDescribed) +HANDLE_DI_FLAG((1 << 30), CxxReturnUdt) rnk wrote: > @dblaikie @aprantl, does this seem like a reasonable flag to add, or shoul

[PATCH] D69743: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood

dblaikie added a comment. What's the extra DWARF that's emitted from the backend? Is there a corresponding LLVM code change/test, or is the functionality there but dormant in this particular case? (& already running/tested in other cases, such as when fn1 is defined in the same CU) CHANGES SI

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3246-3247 DWARFVersion = ExplicitVersion; } + else if (DefaultDWARFVersion != 0) +DWARFVersion = DefaultDWARFVersion; Looks like this should be on a single line to

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3246-3247 DWARFVersion = ExplicitVersion; } + else if (DefaultDWARFVersion != 0) +DWARFVersion = DefaultDWARFVersion; dblaikie wrote: > Looks like this should be on

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3247-3248 } + else if (DefaultDWARFVersion != 0) +DWARFVersion = DefaultDWARFVersion; cmtice wrote: > dblaikie wrote: > > Hmm, actually - why is this case necessary/what d

[PATCH] D69743: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood

dblaikie added a comment. In D69743#1733101 , @vsk wrote: > In D69743#1732967 , @dblaikie wrote: > > > What's the extra DWARF that's emitted from the backend? Is there a > > corresponding LLVM code change/test, or

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added a comment. In D69822#1733226 , @probinson wrote: > > Want to decouple setting the DWARF version from enabling/disabling > > generation of debug info. > > Um, why? If you've got a big build system with various users opting in/out of DWARF

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added a comment. In D69822#1733258 , @aprantl wrote: > Since this sounds like it is hidden inside of some other tooling — is passing > the frontend option `-dwarf-version=5` not an option? "hidden inside of some other tooling" - in the same sen

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added a comment. In D69822#1733262 , @probinson wrote: > In D69822#1733243 , @dblaikie wrote: > > > In D69822#1733226 , @probinson > > wrote: > > > > > > Want to de

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3250 // -gline-directives-only supported only for the DWARF debug info. if (DWARFVersion == 0 && DebugInfoKind == codegenoptions::DebugDirectivesOnly) DebugInfoKind = codegenoptions::NoD

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/include/clang/Driver/Options.td:1955 Flags<[CC1Option]>; +def fdebug_default_version: Joined<["-"], "fdebug-default-version=">, Group; def fdebug_prefix_map_EQ probinson wrote: > If this is specifically the d

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/include/clang/Driver/Options.td:1955 Flags<[CC1Option]>; +def fdwarf_default_version: Joined<["-"], "fdwarf-default-version=">, Group; def fdebug_prefix_map_EQ probinson wrote: > Probably should have HelpText

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me - but maybe give it a day to see if anyone else has further thoughts/that you've addressed their feedback too. Comment at: clang/test/Driver/debug-defau

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

dblaikie added inline comments. Comment at: clang/test/Driver/debug-default-version.c:37 + +// NODEBUGINFO-NOT: "-debug-info-kind=" + Same issue as with the dwarf-version below. It's probably easier to remove the "" (FileCheck arguments aren't quoted - these qu

[PATCH] D69935: [DeclCXX] Remove unknown external linkage specifications

dblaikie added a comment. "The change introduced new external linkage languages ("C++11" and "C++14") which not supported in C++." Could you describe more what you mean by "not supported in C++"? These identifiers aren't part of the C++ standard/they're an implementation detail of the compiler

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

This revision was automatically updated to reflect the committed changes. Closed by commit rG8d8f9c244074: [clang] Add -fdebug-default-version for specifying the default DWARF version (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D69822?vs=228250&id=228287#toc Repos

[PATCH] D69935: [DeclCXX] Remove unknown external linkage specifications

dblaikie added a comment. In D69935#1737615 , @ekatz wrote: > With this change (current trunk) you can write code as follows: > > extern "C++11" int x; > > > And it will pass compilation. No other compiler support it, nor it should, as > there is no suc

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

dblaikie added a comment. The failure I am investigating from the original commit of this at Google probably isn't related to the assertion failure that caused the revert of this patch/being addressed by this recommit. So if you could hold off a bit while I try to help provide a reproduction or

[PATCH] D69935: [DeclCXX] Remove unknown external linkage specifications

dblaikie added a comment. In D69935#1738273 , @uabelho wrote: > Don't you need to also remove > > case LinkageSpecDecl::lang_cxx_11: > case LinkageSpecDecl::lang_cxx_14: > > > from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp?

[PATCH] D69935: [DeclCXX] Remove unknown external linkage specifications

dblaikie added a comment. In D69935#1739235 , @dblaikie wrote: > In D69935#1738273 , @uabelho wrote: > > > Don't you need to also remove > > > > case LinkageSpecDecl::lang_cxx_11: > > case LinkageSpecDecl::lang_

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

dblaikie added a comment. Yeah, I'm OK with warning on the deprecated cases (especially for symmetry with GCC's warning) - not sure why that didn't occur to me/why I didn't mention it in my previous comment... Deprecation wording is, in C++17, 15.8.1 [class.copy.ctor] p6, and 15.8.2 [class.cop

[PATCH] D69935: [DeclCXX] Remove unknown external linkage specifications

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69935/new/ https://reviews.llvm.org/D69935 ___ cfe-commits mailin

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

dblaikie added a comment. In D68185#1742809 , @xbolva00 wrote: > Ah, Clang has it under -Wdeprecated. > https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated. > > But combo -Wall -Wextra does not enable this warning, sadly. > > This should be

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

dblaikie added inline comments. Comment at: clang-tools-extra/clang-move/tool/ClangMove.cpp:113 move::MoveDefinitionSpec Spec; - Spec.Names = {Names.begin(), Names.end()}; + Spec.Names = (std::vector &)Names; Spec.OldHeader = OldHeader; rnk wrote: > Conve

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

dblaikie added a comment. In D69970#1742575 , @vsk wrote: > @dblaikie do you need more time to investigate? fwiw I didn't uncover any new > failures in a stage2 build with this patch applied. I haven't figured this out yet, sorry - bit inundated with a

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

dblaikie added a comment. I can confirm we still have the same internal failure with this revised/updated patch. Trying to reduce/reproduce it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69970/new/ https://reviews.llvm.org/D69970 ___ c

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

dblaikie added a comment. So I /think/ what's happening here is the extra addresses needed for the call_site low_pcs are creating new basic blocks (or whatever the logic is in the backend that says "use line zero at the start of each basic block-like thing" - possible that logic is overly conse

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:905 + + const SmallVector &operator=(const std::vector &Vec) { +this->assign(Vec.begin(), Vec.end()); aganea wrote: > rnk wrote: > > +@dblaikie, who knows more about STL containe

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:905 + + const SmallVector &operator=(const std::vector &Vec) { +this->assign(Vec.begin(), Vec.end()); aganea wrote: > dblaikie wrote: > > aganea wrote: > > > rnk wrote: > > > > +

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

dblaikie added subscribers: probinson, echristo. dblaikie added a comment. OK - I believe the issue I'm seeing is an internal issue, a fragile/buggy test case. Please go ahead with this change if (by the sounds of it) all other concerns are addressed. For more rambling context: A test case woul

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

dblaikie added a comment. In D69970#1745038 , @vsk wrote: > Don't emit declaration subprograms for functions with reserved names. There > may not be much benefit from referencing these functions from call site tags > (e.g. instead of relying on call site

[PATCH] D70219: Make `-fmodule-file==` apply to .pcm file compilations

dblaikie added a comment. Looks reasonable to me, but I don't know a great deal about the PrebuiltModuleFiles & so I'd like @rsmith to at least glance at/approve this. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70219/new/ https://reviews.llvm.org/D70219 __

[PATCH] D70340: Add a key method to Sema to optimize debug info size

dblaikie added a comment. In D70340#1748712 , @thakis wrote: > I don't see any reason not to do this. What's there to discuss? I'm probably > missing something obvious. Eh, it's a bit quirky - adds production code (albeit a very small amount) only to i

[PATCH] D70111: [DWARF5]Addition of alignment field in the typedef for dwarf5

dblaikie added a comment. I have some vague recollection that the DWARF C bindings didn't have stability guarantees? Does that sound familiar to anyone? What sort of changes have been made to them in the past? Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:804-810 + /

[PATCH] D70111: [DWARF5]Addition of alignment field in the typedef for dwarf5

dblaikie added a comment. Please update the go bindings and include a release note that this API is changing & why. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. Generally if you're not touching LLVM code, I would suggest not adding an LLVM test - unless this is new or surprising usage of existing LLVM functionality (or the functionality was otherwise undertested in LLVM previously). But I'm guessing that's not the case here?

[PATCH] D70111: [DWARF5]Addition of alignment field in the typedef for dwarf5

dblaikie added a comment. In D70111#1754702 , @awpandey wrote: > I am considering this > > - Will commit this without C bindings > - will give separate patch for C bindings and release notes (if necessary). > - will give another patch for Go bindings and r

[PATCH] D70342: [Diagnostics] Put "deprecated copy" warnings into -Wdeprecated-copy

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me - can handle anything else from @rsmith in post-commit review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70342/new/ htt

[PATCH] D70625: [DebugInfo][BPF] Support to emit debugInfo for extern variables

dblaikie added a comment. Seems to have a few things going on - the title talks about global variables - the description talks about extern types (guessing that's just a typo?) - the patch itself seems to have code that's visiting more functions? ( processFuncPrototypes ) - & also I'd generally

[PATCH] D70111: [DWARF5]Addition of alignment field in the typedef for dwarf5

dblaikie added a comment. Looks OK to me - please recommit when you're ready. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llv

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1765637 , @probinson wrote: > For the case: > > cat def.c > int global_var = 2; > > > def.o should have debug info for the definition of global_var. > For the case: > > cat noref.c > extern int global_va

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1765714 , @yonghong-song wrote: > @dblaikie Good points. I will guard external variable debug info generation > under `-fstandalone-debug` flag. Oh, I figured given your needs this'd be guarded behind the target bein

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1765675 , @yonghong-song wrote: > @probinson for the question, > > > Does bpf require debug info for the declaration of global_var in noref.c ? > > No, bpf only cares the referenced external global variables. So my curr

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1765784 , @probinson wrote: > In D70696#1765671 , @dblaikie wrote: > > > In D70696#1765637 , @probinson > > wrote: > > > > > For the case

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1765789 , @dblaikie wrote: > In D70696#1765784 , @probinson wrote: > > > In D70696#1765671 , @dblaikie > > wrote: > > > > > In D70696#176

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1765823 , @SouraVX wrote: > @dblaikie , If you try switching to C compiler. then the DW_TAG_variable is > their. for C++ . It's not generated. Please check godbolt again. -- though > strange Hmm - I don't seem to s

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1761514 , @awpandey wrote: > Hi @aprantl and @dblaikie. Currently, there is no test case for the > unspecified type, so I have added that in the regression test suite. It looks to me like there are a few tests for un

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. Many of the test cases could be collapsed into one file - using different variables that are used, unused, locally or globally declared, etc. Is this new code only active for C compilations? (does clang reject requests for the bpf target when the input is C++?) I ask d

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

dblaikie added a comment. I feel like there's something missing about this bug/issue - is there a good explanation/understanding for why does the bug only occur with the two levels of static member inline variable templates & not one? Perhaps there's some existing code that works for simple cas

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1770330 , @awpandey wrote: > > It looks to me like there are a few tests for unspecified_type already: > > > > $ grep -r unspecified_type llvm/test > > llvm/test/Assembler/debug-info.ll:; CHECK-NEXT: !7 = !DIBasicType(

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1771709 , @probinson wrote: > In D70524#1771522 , @shafik wrote: > > > @probinson I was reading the C++ "auto" return type > >

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a comment. @rsmith - thanks for looking at this. I'd looked at it a bit and I was/am still wondering whether there's a way to coalesce these codepaths between PCH and the existing modular code generation support? But if you're good with it, that's certainly good enough for me.

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1772690 , @probinson wrote: > In D70524#1772117 , @dblaikie wrote: > > > In D70524#1771709 , @probinson > > wrote: > > > > > In D70524#17

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1772961 , @probinson wrote: > > Perhaps we should implement this mode under -fno-standalone-debug (or > > something more aggressive) since "standalone" is one of the only places I > > can think of where having the full

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D70696#1774931 , @Jac1494 wrote: > For some real life case like below we need debuginfo for declaration of > global extern variable . > > $cat s

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added inline comments. Comment at: clang/test/CodeGen/debug-info-extern-basic.c:14-23 +extern int (*foo)(int); +int test3() { + return foo(0); +} + +int test4() { + extern int (*foo2)(int); yonghong-song wrote: > dblaikie wrote: > > What do these tests

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a comment. > In D69778#1772125 , @dblaikie wrote: > >> I was/am still wondering whether there's a way to coalesce these codepaths >> between PCH and the existing modular code generation support? > > > In what way could this be united more

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1779179 , @awpandey wrote: > @dblaikie . I have removed the redundant test case. What else should I do in > this patch? Please address this warning before committing: /usr/local/google/home/blaikie/dev/llvm/src/cl

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1783363 , @probinson wrote: > > Hmm, maybe this feature/suggestion is broken or at least not exactly > > awesome when it comes to auto-returning functions that are eventually > > void-returning functions? Now the funct

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a comment. Do you have any particular users/use case for this? Might be best as two separate patches (one for the LLVM change, one for the Clang change) with tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71451/new/ https://r

[PATCH] D71508: [DebugInfo] Duplicate file names in debug info

dblaikie added a comment. In D71508#1786212 , @kamleshbhalui wrote: > In D71508#1786148 , @probinson wrote: > > > In D71508#1786122 , @kamleshbhalui > > wrote: > > > > > In

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

dblaikie added a comment. Yeah, this is working around a more fundamental representational issue. Like member function templates, member variable templates shouldn't be pre-emptively built or ever appear in the "members" list of the type (because that list isn't closed - we don't know what inst

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

dblaikie added subscribers: JDevlieghere, aprantl. dblaikie added a comment. Yeah, GCC's debug info for this is... not good. I don't think it's anything we'd need to be inspired by. Here's the tags and names: DW_TAG_compile_unit DW_AT_name ("templ2.cpp") DW_TAG_structure_type D

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

dblaikie added a comment. In D70537#1788252 , @probinson wrote: > @dblaikie let me reflect this back to make sure I get it: > Template members (methods or variables) would never appear in the *metadata* > description of the struct; but metadata descripti

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. In D70524#1787566 , @SouraVX wrote: > > It looks like this implementation is a bit buggy in one way and incomplete > > in another: > > > > 1. even if the auto-returning function is defined, that function definition > > doesn't

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a comment. In D71451#1787729 , @Jac1494 wrote: > In D71451#1786517 , @dblaikie wrote: > > > Do you have any particular users/use case for this? > > > A case when shared library built without debug in

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a comment. In D71451#1791314 , @Jac1494 wrote: > >> @Jac1494 - have you made any measurements of the size increase of this > >> change? Perhaps a self-host build of clang? > > With change(default) and without change size diffrence given bel

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a subscriber: JDevlieghere. dblaikie added a comment. In D71451#1793828 , @Jac1494 wrote: > > Am I reading this right that the data would suggest that enabling this > > feature /reduces/ the size of debug info sections? That doesn't sound r

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1794440 , @rnk wrote: > Three of these tests have been failing for me locally since this patch landed: > > Failing Tests (3): > Clang :: CodeGen/debug-info-extern-basic.c > Clang :: CodeGen/debug-info-exter

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1795302 , @yonghong-song wrote: > > Could you link to particular bots that have logs showing they ran this > > test? (perhaps the logs have been retired by now, though - since this patch > > was reverted :/ - but then

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

dblaikie added a comment. In D70696#1795427 , @yonghong-song wrote: > @dblaikie The root cause has been identified by @rnk . A clang plugin is used > which requires the following patch: > > > https://github.com/llvm/llvm-project/commit/5128026467cbc17

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

dblaikie added inline comments. Comment at: clang/lib/Serialization/ModuleManager.cpp:183 // Get a buffer of the file and close the file descriptor when done. - Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false); + Buf = FileMgr.getBufferForFil

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

dblaikie added a comment. Could you upload this with full context ( https://llvm.org/docs/Phabricator.html#id4 mentions how to do this)? Does this still have an OpenMP special case? The production code changes don't seem to mention OpenMP anymore, but there's still a lot of test updates for Op

[PATCH] D73846: [PCH] make sure to not warn about unused macros from -D

dblaikie added a comment. Looks pretty reasonable to me - but I don't know enough about the macro and source location infrastructure. @rsmith - mind taking a quick look? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73846/new/ https://reviews.llvm.org/D73846

[PATCH] D76269: [opaque pointer types] Remove deprecated Instruction/IRBuilder APIs.

dblaikie added a comment. Probably worth committing the fixes separately from the API removal (so if the removal has to be rolled back the fixes can be left as-is). @t.p.northover - any thoughts on the removal? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a subscriber: rnk. dblaikie added a comment. In D69778#1779042 , @llunak wrote: > In D69778#1776472 , @dblaikie wrote: > > > I guess one aspect is that -building-pch-with-obj seems like it duplicates

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a comment. In D69778#1927761 , @llunak wrote: > In D69778#1927526 , @dblaikie wrote: > > > @rnk - know anything about the history of -building-pch-with-obj and > > whether it could be replaced/merge

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a comment. In D69778#1928417 , @llunak wrote: > In D69778#1928111 , @dblaikie wrote: > > > In D69778#1927761 , @llunak wrote: > > > > > It comes from 08c5a7b8f

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a comment. In D69778#1929817 , @llunak wrote: > In D69778#1929737 , @dblaikie wrote: > > > I'm not sure how that could be possible - there's no data transferred > > between the compilation of the TU'

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added inline comments. Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1808-1809 Record.push_back(VE.getMetadataOrNullID(N->getType())); + if (M.getDwarfVersion() >= 5) +Record.push_back(N->getDefault()); Record.push_back(VE.getMetadataOrNullID(N->getVal

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

dblaikie added a comment. I don't think the more targeted fix is a good thing - is the same problem of not demangling names not there for the other cases that the original fix addressed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73282/new/ https://reviews.llvm.org/D73282 __

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added a comment. Might be easier in smaller patches - Bitcode & IR support, then LLVM/codegen emission and Clang/IR gen. > I have to update dityperefs-3.8.ll and dityperefs-3.8.ll.bc because they are > no longer compatible with this patch. The bitcode files in test/Bitcode generally s

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D73282#1856076 , @shafik wrote: > In D73282#1855888 , @dblaikie wrote: > > > I don't think the more targ

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-template-parameter.cpp:30-32 + foo f1; + foo f2; + foo<> f3; What are these three (6 if you include the fact that each foo has one default template parameter still) testing? Could it

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added a comment. @aprantl - functionality looks fine to me, could you check/sign off on the bitcode backwards compatibility stuff? (& ultimately this should probably be committed in 3 patches - one for the IR support, then one for Clang to generate that IR, and another for LLVM to use

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a comment. If this feature treats a declaration in a header different from one in a primary source file - I think that's problematic from a debug info/debugging perspective (& perhaps a quirk of how this functionality was originally implemented for the kernel feature thingy (the

[PATCH] D73261: [dwarf5] Support DebugInfo for constexpr for C++ variables and functions

dblaikie added a comment. In D73261#1868790 , @djtodoro wrote: > @awpandey Thanks for doing this, but could you please explain the motivation > of implementing this? +1 (as I asked earlier & don't think there's been a response for) CHANGES SINCE LAST

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a comment. In D71451#1828647 , @Jac1494 wrote: > > Do you have any reason to believe these patches would reduce the size of > > the debug info? It seems like they only add more debug info, and don't > > remove anything - so we'd expect an

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3659 // Use llvm function name. -Name = Fn->getName(); +if (Fn->getName().startswith("___Z")) + LinkageName = Fn->getName(); aprantl wrote: > aprantl wrote: > > Could

[PATCH] D72331: OpaquePtr: add type to inalloca attribute.

dblaikie added a comment. @rnk mind taking a look at this when you get a chance just to sanity check from the inalloca design side of things? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72331/new/ https://reviews.llvm.org/D72331 ___ cfe

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2155 friend class MDNode; + bool isDefault; What about default arguments for non-type template parameters? ("template" etc... ) Repository: rG LLVM Github Monorepo CH

[PATCH] D73261: [dwarf5] Support DebugInfo for constexpr for C++ variables and functions

dblaikie added a comment. In D73261#1838683 , @probinson wrote: > In D73261#1838552 , @djtodoro wrote: > > > > Is it necessary to use DIFlags? I am willing to do that but generally, it > > > is not welcomed because

[PATCH] D73261: [dwarf5] Support DebugInfo for constexpr for C++ variables and functions

dblaikie added a comment. General question: What purpose do you (or anyone else - @probinson, @aprantl) have for this attribute? I'd like to encourage the idea that we don't need to implement every feature DWARF provides if there's no planned use for it - things can/should be implemented as-mot

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

dblaikie added inline comments. Comment at: clang/lib/Sema/Sema.cpp:985-986 + +// FIXME: Instantiating implicit templates already in the PCH breaks some +// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 . +if(LangOpts.PCHInstantiateTemplates && !Lang

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3659 // Use llvm function name. -Name = Fn->getName(); +if (Fn->getName().startswith("___Z")) + LinkageName = Fn->getName(); shafik wrote: > dblaikie wrote: > > aprant

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

dblaikie added a comment. Yeah, in general I think special casing a particular name mangling is probably not a great idea (what about in Microsoft mode with a different mangling scheme?), so I'm somewhat inclined towards doing this in general as @aprantl was suggesting. CHANGES SINCE LAST ACT

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2134 protected: + bool isDefault; + Rename this (& the ctor/other parameters) to "IsDefault" to conform to LLVM's naming conventions ( https://llvm.org/docs/CodingStandards.h

[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

dblaikie added inline comments. Comment at: llvm/lib/AsmParser/LLParser.cpp:4858 /// ::= !DITemplateValueParameter(tag: DW_TAG_template_value_parameter, -/// name: "V", type: !1, value: i32 7) +/// name: "V", type

<    8   9   10   11   12   13   14   15   16   17   >