[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/include/clang/Driver/ToolChain.h:495 + // compilation uses DWARF5. + virtual unsigned GetAdjustedDwarfVersion(unsigned v) const { return v; } + Given these semantics (pass/return the version) maybe "AdjustDwarfV

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3957 + RenderDebugEnablingArgs(Args, CmdArgs, DebugInfoKind, + std::min(DWARFVersion, TC.getMaxDwarfVersion()), DebuggerTuning);

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

dblaikie added a comment. I've committed 2e83ccc2ee89110659f3cb313968a0c970d4 which takes parts of this patch - specifically fixing the test that was already intended to test the RefCountedBase's copy constructor, but did

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3957 + RenderDebugEnablingArgs(Args, CmdArgs, DebugInfoKind, + std::min(DWARFVersion, TC.getMaxDwarfVersion()), DebuggerTuning);

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added subscribers: scott.linder, JDevlieghere. dblaikie added inline comments. Comment at: clang/test/Driver/cuda-unsupported-debug-options.cu:22 +// Make sure we do not see any dwarf version other than 2, regardless of what's used on the host side. +// CHECK-NOT: {{-dw

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

dblaikie added inline comments. Comment at: llvm/include/llvm/Support/ManagedStatic.h:25 +// that are const with no params. +template struct HasRetainRelease { +private: Are there many uses that rely on this? I don't think it's really worth all this infrastruct

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

dblaikie added inline comments. Comment at: llvm/include/llvm/Support/ManagedStatic.h:25 +// that are const with no params. +template struct HasRetainRelease { +private: njames93 wrote: > dblaikie wrote: > > Are there many uses that rely on this? I don't think i

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/test/Driver/debug-options.c:364-366 +// GEMBED_2: warning: debug information option '-gembed-source' is not supported for target // NOGEMBED_5-NOT: "-gembed-source" +// NOGEMBED_2-NOT: warning: debug information option '-gemb

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

dblaikie accepted this revision. dblaikie added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:155 public: - TrueMatcherImpl() { -Retain(); // Reference count will never become zero. - } + TrueMa

[PATCH] D90507: [Driver] Add DWARF64 flag: -gdwarf64

dblaikie added inline comments. Herald added a subscriber: hoy. Comment at: clang/include/clang/Basic/CodeGenOptions.def:35 CODEGENOPT(AsmVerbose, 1, 0) ///< -dA, -fverbose-asm. +CODEGENOPT(Dwarf64 , 1, 0) ///< -gdwarf64. CODEGENOPT(PreserveAsmComments, 1, 1)

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

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks like GCC has changed the semantics here despite the backwards compatibility break - as much as I'd rather not break backwards compatibility it's probably best on balance to maintain

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:296 +def warn_drv_dwarf_version_limited_by_target : Warning< + "debug information option '%0' is not supported. It needs DWARF-%2 but target '%1' only provides DWARF-%3.">, + InGroup

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

dblaikie added a comment. In D80391#2438447 , @MaskRay wrote: > In D80391#2437926 , @dblaikie wrote: > >> Looks like GCC has changed the semantics here despite the backwards >> compatibility break - as much as I'd

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

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3718 + } else +DwarfFission = DwarfFissionKind::None; MaskRay wrote: > dblaikie wrote: > > Rather than undoing the DwarfFission uinitialization, instead could the > > DwarfF

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/test/Driver/cuda-unsupported-debug-options.cu:18 +// RUN: %clang -### -target x86_64-linux-gnu -c %s -gdwarf-5 -gembed-source 2>&1 | FileCheck %s --check-prefix=DWARF-CLAMP +// CHECK: debug information option '{{-gz|-fdebug-info

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-static-member.cpp:144-147 +// For some reason, const_va is not emitted when the target is MS. +// NOT-MS: !DIDerivedType(tag: DW_TAG_member, name: "const_va", +// NOT-MS-SAME: line: [[@LINE-3]]

[PATCH] D78658: [clang][Frontend] Add missing error handling

dblaikie accepted this revision. dblaikie added a comment. Looks good to me - thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78658/new/ https://reviews.llvm.org/D78658 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lis

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie reopened this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sorry about the delays in review - but please revert this patch until we can hash out a few more details. I really don't think this is the best direction forward for a core abstraction & I

[PATCH] D89500: Fix the error message with -fbasic-block-sections=list=

dblaikie added a comment. In D89500#2344234 , @davezarzycki wrote: > I think I fixed it. Please verify: 87f6de72bcd346bbbf468e9f9a0e9d1bbf0630a9 > Thanks - if that's unblocked you, gr

[PATCH] D78658: [clang][Frontend] Add missing error handling

This revision was automatically updated to reflect the committed changes. Closed by commit rGfd14a1f6fff3: [clang][Frontend] Add missing error handling (authored by LemonBoy, committed by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78658

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

dblaikie added a comment. In D90010#2352556 , @njames93 wrote: > Isn't `llvm::errs()` buffered, negating most of the benefit here. +1 to this (the patch description doesn't explain any specific motivation either - whether it's performance (runtime? memor

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

dblaikie added a comment. In D80531#2353268 , @njames93 wrote: > In D80531#2352944 , @kkleine wrote: > >> @dblaikie sorry for the late feedback. The `LLVM_ENABLE_WERROR:BOOL` will >> "Stop and fail the build, if a

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

dblaikie added a comment. In D90010#2355421 , @Hiralo wrote: > In D90010#2352556 , @njames93 wrote: > >> Isn't `llvm::errs()` buffered, negating most of the benefit here. > > If it is buffered, we would expect singl

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

dblaikie added a comment. In D90010#2355443 , @Hiralo wrote: > In D90010#2355432 , @dblaikie wrote: > >> Looks like you might be able to do something like >> "llvm::errs().setBuffered()" ? > > Do we need to set it

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

dblaikie added a comment. In D90010#2355489 , @Hiralo wrote: > In D90010#2355460 , @dblaikie wrote: > >> In D90010#2355443 , @Hiralo wrote: >> >>> In D90010#2355432

[PATCH] D89893: [Clang] [TableGen] Clean up !if(!eq(boolean, 1) and related booleans

dblaikie added subscribers: mehdi_amini, dblaikie. dblaikie added a comment. @lattner minor detail/annoyance: Phabricator doesn't email the mailing lists if a review is "approved" with no comments. To ensure the approval is recorded on the list, please include some text (a stub "." would suffice

[PATCH] D89893: [Clang] [TableGen] Clean up !if(!eq(boolean, 1) and related booleans

dblaikie added a comment. In D89893#2357480 , @Paul-C-Anagnostopoulos wrote: > I'll remember this, too. > > @dblaikie How did you happen to notice? Reading the mailing list & seeing the phab review and commit without any emails from anyone else in the r

[PATCH] D89794: [SyntaxTree] Implement "by-pointer output parameter to return value" refactoring.

dblaikie added a comment. I imagine test coverage would be handy (I'm probably not the right reviewer for this in general, but was/am curious to see how it goes - you might want to search for other contributors/reviewers to the code you're changing and request/add them as reviewers for this pat

[PATCH] D83448: [CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators

dblaikie added a comment. Idle curiosity: The patch description says "non-trivial C struct temporaries" - can C have non-trivial structs? Or is this limited to Objective C (judging/guessing by the tests)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D89500: Fix the error message with -fbasic-block-sections=list=

dblaikie added a comment. @tmsriram ping on the follow-up here Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89500/new/ https://reviews.llvm.org/D89500 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D83448: [CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators

dblaikie added a comment. In D83448#2357574 , @ahatanak wrote: > C structs with ObjC pointer fields are non-trivial when ARC is enabled since > construction, destruction, and copy are non-trivial (see > https://reviews.llvm.org/D41228). As far as I know,

[PATCH] D90109: [clang-tidy] Use ANSI escape codes for --use-color on Windows

dblaikie added a comment. In D90109#2356317 , @aaron.ballman wrote: > In D90109#2352180 , @dsanders11 > wrote: > >> Added a few inline comments for clarification. >> >> **Note**: I was not able to get a debug buil

[PATCH] D88603: [WebAssembly] Add support for DWARF type units

dblaikie added a comment. In D88603#2357973 , @dschuff wrote: > This broke the bots for some strange reason that didn't reproduce locally. > But because it was ~all of them, I probably just did something stupid. Good to have links to buildbots and/or quo

[PATCH] D88603: [WebAssembly] Add support for DWARF type units

dblaikie added a comment. In D88603#2360845 , @dschuff wrote: > @sbc100 I found that the cause of the assertion is that > in dwarf 5, the type units apparently go in the .debug_info section (instead > of the .debug_type section), and this section already

[PATCH] D88603: [WebAssembly] Add support for DWARF type units

dblaikie added a comment. In D88603#2361010 , @dblaikie wrote: > In D88603#2360845 , @dschuff wrote: > >> @sbc100 I found that the cause of the assertion is that >> in dwarf 5, the type units apparently go in the .

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

dblaikie added a comment. I imagine, again, the idea is that if you're generating that many warnings that the performance of printing warnings matters, perhaps you're not paying attention to those warnings? you could disable the ones you aren't interested in? Especially if they're being printed

[PATCH] D100630: [Debug-Info][DBX] DW_TAG_rvalue_reference_type should not be generated when dwarf version is smaller than 4

dblaikie added a comment. In D100630#2755936 , @aprantl wrote: > Doing this in the backend SGTM, assuming all of @dblaikie's comments are > addressed. The complication is that if we naively lower DW_TAG_rvalue_reference to DW_TAG_reference, then we'll

[PATCH] D99160: [X86][FastISEL] Support DW_TAG_call_site_parameter with FastISEL

dblaikie added a comment. ~5% object size growth (probably more heavily weighted towards object size when using Split DWARF (most of the growth is likely in the .debug_addr/rela.debug_addr in the .o file, rather than in the DIEs in the .dwo file)) seems like enough that I wouldn't really be sup

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. > I took another look. I think the divergence comes from > getAs vs hasPrototype. The debug data generation uses > hasPrototype while getAs is used as overloadable attribute > processing as long as unique linkage name processing before this change. More > specifically

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

dblaikie added a comment. This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-

[PATCH] D100671: [ADT] Factor out in_place_t and expose in Optional ctor

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Probably skip the other two in_place_* until we have a use for them. Comment at: llvm/include/llvm/ADT/STLForwardCompat.h:53-73 + +template +struct

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

dblaikie added a comment. In D102356#2758371 , @hoy wrote: > In D102356#2758179 , @dblaikie > wrote: > >> This was previously crashing, I guess? Testing should validate the behavior >> beyond the crash, though -

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

dblaikie added a comment. In D102356#2761010 , @hoy wrote: > In D102356#2760973 , @dblaikie > wrote: > >> In D102356#2758371 , @hoy wrote: >> >>> In D102356#2758179

[PATCH] D100630: [Debug-Info][DBX] DW_TAG_rvalue_reference_type should not be generated when dwarf version is smaller than 4

dblaikie added a comment. This isn't the sort of thing I'd like to leave to a FIXME later - seems like the sort of thing we shouldn't create now to fix later. @aprantl mind having a second look at this to consider the situation further? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added subscribers: JDevlieghere, rjmccall. dblaikie added a comment. OK - poked around a bit more to better understand this, so attempting to summarize my current understanding (excuse the repetition from previous parts of this thread) and results. - `__attribute__((overloadable))` can

[PATCH] D102654: [DebugInfo][test] Check specific func name to ignore codegen differences

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-line.cpp:149 -// CHECK-LABEL: define +// CHECK-LABEL: f11 __complex double f11() { Probably flesh that out a bit. Maybe `define{{.*}}f11`? Repository: rG LLVM Github Monorepo CHA

[PATCH] D102638: [NFC] Pass GV value type instead of pointer type to GetOrCreateLLVMGlobal

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102638/new/ https://reviews.llvm.org/D102638

[PATCH] D102654: [DebugInfo][test] Check specific func name to ignore codegen differences

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102654/new/ https://reviews.llvm.org/D102654

[PATCH] D102728: [clang][Sema] removes -Wfree-nonheap-object reference param false positive

dblaikie added a comment. Thanks for following up on this. - Probably best to commit the revert of the workarounds as a separate follow-up commit, but I appreciate seeing them here to understand that this patch addresses them - Comment at: clang/lib/Sema/SemaChecking.cpp:104

[PATCH] D102782: Add support for Warning Flag "-Wstack-usage="

dblaikie added a comment. Generally looks reasonable to me - but I'll leave signoff to some other folks who have been more involved in the discussion so far. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102782/new/ https://reviews.llvm.org/D10278

[PATCH] D102568: [Driver] Delete -mimplicit-it=

dblaikie added a comment. > I think "waiting for a few releases" is too much and doesn't improve things > (they will notice issues until you remove the option). I can accept "waiting > for one major release". Having fairly broad windows of not breaking backwards compatibility is a fairly reaso

[PATCH] D102568: [Driver] Delete -mimplicit-it=

dblaikie added a comment. In D102568#2769341 , @MaskRay wrote: > I think libvpx's ASFLAGS usage is about GNU as, not the driver option. > > In D102568#2769296 , @dblaikie > wrote: > >>> I think "waiting for a few

[PATCH] D102568: [Driver] Delete -mimplicit-it=

dblaikie added a comment. In D102568#2769395 , @MaskRay wrote: > In D102568#2769389 , @dblaikie > wrote: > >> In D102568#2769341 , @MaskRay >> wrote: >> >>> I think libv

[PATCH] D100630: [Debug-Info][DBX] DW_TAG_rvalue_reference_type should not be generated when dwarf version is smaller than 4

dblaikie added a comment. I'd still like to hear from some of the other folks mentioned previously. (though there is precedent for debug info modes being frontend, like -gmlt, etc - so it's not totally novel/to be avoided, but I'd like the discussion first) Repository: rG LLVM Github Monorep

[PATCH] D100630: [Debug-Info][DBX] DW_TAG_rvalue_reference_type should not be generated when dwarf version is smaller than 4

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D100630#2775207 , @aprantl wrote: > If gracefully lowering rvalue references to references needs knowledge about > the Clang typesystem (or dep

[PATCH] D102650: Old work on P0388. For reference in D102645. Not for review / commit.

dblaikie added a comment. FWIW, there's some way to post reviews in draft form (I think via arc --draft or something like that) - which would ensure that emails for the review aren't sent to the -commits mailing list for something you don't want folks on the -commits list to review just yet, et

[PATCH] D102650: Old work on P0388. For reference in D102645. Not for review / commit.

dblaikie added subscribers: MaskRay, mehdi_amini. dblaikie added a comment. In D102650#2779193 , @urnathan wrote: > dblaikie, do you know if there's a way to do it outside of arc? (for reasons, > I've not got arc to work from inside work). I tried not s

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. Looks like GCC emits aliases as a `DW_TAG_variable` without a location, not as a `DW_TAG_imported_declaration` - what gave you the inspiration to do it in this way? (I think it's probably good, but DWARF doesn't lend itself to novelty so much... can be good to stick wi

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

dblaikie added subscribers: jhenderson, probinson, dblaikie. dblaikie added a comment. Can't say I'm super enthusiastic about this (I assume the build already supports prefixes and suffixes, which I'd hope would be adequate - but presumably are not for your use case), though there's some, I thin

[PATCH] D101566: Let -Wweak-template-vtables warn on implicit instantiations

dblaikie added a comment. In D101566#2785190 , @aaronpuchert wrote: > In D101566#2734948 , @dblaikie > wrote: > >> Makes it hard to justify the complexity in the compiler if it's hard to >> justify/support the

[PATCH] D101566: Let -Wweak-template-vtables warn on implicit instantiations

dblaikie added a comment. In D101566#2786112 , @aaronpuchert wrote: > In D101566#2785271 , @dblaikie > wrote: > >> Right - to remove -Wweak-template-vtable in its entirety. The original >> implementation explic

[PATCH] D103131: support debug info for alias variable

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4957 + auto Loc = getLineNumber(D->getLocation()); + DBuilder.createGlobalVariableExpression( + DContext, Name, StringRef(), Unit, Loc, Ty, probinson wrote: > I wasn't saying tha

[PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

dblaikie added a comment. In D101921#2786245 , @MaskRay wrote: > Because of `new MCObjectFileInfo`, we cannot use a forward declaration > (incomplete class) to replace `#include "llvm/MC/MCObjectFileInfo.h"` in > `TargetRegistry.h`. > > I thought about

[PATCH] D103465: [OpaquePtr] Track pointee types in Clang

dblaikie added a comment. The changes look like the right direction to me - though I don't know/couldn't confirm whether more changes will be needed in other places. Comment at: clang/lib/CodeGen/Address.h:29-30 public: Address(llvm::Value *pointer, CharUnits alignment) -

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2789493 , @probinson wrote: >> Mixed feelings - somewhat in favor of "do the thing that's probably already >> fairly tested/known to work" (GCC's thing). But open to the idea that that >> approach has problems, for s

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2792159 , @probinson wrote: > In D103131#2791881 , @dblaikie > wrote: > >> In D103131#2789493 , @probinson >> wrote: >> Mixed f

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. Thanks for taking a look, @rjmccall - I really appreciate it! Sorry I'm doing a bad job describing (admittedly I'm pretty confused about all this, which is most of the issue) the issue - I'll try to clarify as best I can. In D98799#2781200

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2793775 , @ldionne wrote: > Hello! There are still some false positives, for example this one is breaking > the libc++ CI: > https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a88

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. In D98799#2794366 , @ahatanak wrote: > I don't know know why these fake FunctionDecls are needed, but it looks like > it's okay to avoid creating them. I see a few debug info tests failing if > `GlobalDecl()` instead of a fake f

[PATCH] D103549: [POC] Put annotation strings into debuginfo.

dblaikie added a comment. Generally arbitrary strings are best avoided where possible owing to lack of structure, type safety, semantics, etc. But they might be suitable here since they're opaque to everything from the frontend to the backend. As for supporting it in DWARF, probably with a cust

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. In D98799#2794546 , @ahatanak wrote: > Yes, I think I can fix that. Thanks a lot! If you could follow-up here when that's committed, I can follow-up with a fix/improvement for the debug info mangling issue we've been discussing

[PATCH] D103549: [POC] Put annotation strings into debuginfo.

dblaikie added a comment. In D103549#2794462 , @yonghong-song wrote: >> As for supporting it in DWARF, probably with a custom attribute >> (DW_AT_BTF_annotation? (or "LLVM" instead of "BTF" perhaps, I'm not sure)) >> with a standard form (DW_FORM_strp/

[PATCH] D95202: ADT: Use 'using' to inherit assign and append in SmallString

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - some optional feedback on the code fixes. Please run clang-format on the changes here (phab lint picked up some cases in the test code that could be cleaned up). ==

[PATCH] D95459: Add helper functionality for parsing different attribute syntaxes in arbitrary order

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me! (welcome to wait for more eyes, if you like) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95459/new/ https://reviews.llvm.org/D95459 __

[PATCH] D95772: [DebugInfo] Normalize paths by removing unnecessary dots

dblaikie added a subscriber: rnk. dblaikie added a comment. Does this address @rnk's feedback about symlinks? ( https://reviews.llvm.org/D87657#2296028 ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95772/new/ https://reviews.llvm.org/D95772 ___

[PATCH] D95911: [Docs] Add some documentation for constructor homing, a debug info optimization (-fuse-ctor-homing)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds alright to me. (non-action idle thoughts: Might be worth revisiting this documentation to make it a bit more direct/clearer that many of these optimizations (at least the vtable an

[PATCH] D95746: clang: Exclude efi_main from -Wmissing-prototypes

dblaikie added a comment. Please include test coverage Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95746/new/ https://reviews.llvm.org/D95746 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location

dblaikie added a comment. @MaskRay Sounds like this should be reverted (& that revert picked up by the 12 release branch) & some investigation can continue after that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94735/new/ https://reviews.llvm.o

[PATCH] D96044: DebugInfo: Emit "LocalToUnit" flag on local member function decls.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good. Out of curiosity, how'd you come across this/do you have any particular use case that uses DW_AT_external? Comment at: clang/test/CodeGenCXX/debug-info-clas

[PATCH] D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location

dblaikie added a comment. In D94735#2547370 , @jdoerfert wrote: > In D94735#2546968 , @dblaikie wrote: > >> This patch is about a clang synthesized `struct`. >> `clang/lib/CodeGen/CGOpenMPRuntime.cpp:3463` has a sy

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie created this revision. dblaikie added reviewers: aaron.ballman, rtrieu. dblaikie requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. This improves diagnostic (& important to me, DWARF) accur

[PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. Thanks for the suggestions/details, @dexonsmith - I've posted to llvm-dev here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and will wait for some follow-up (or dead silence) before starting along probably your latter suggestion. Repository:

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie added a comment. In D110044#3009201 , @aaron.ballman wrote: >> This improves diagnostic (& important to me, DWARF) accuracy > > FWIW, I don't think the diagnostic particularly needs more accuracy here -- I > think users know what `nullptr_t` ty

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie updated this revision to Diff 373768. dblaikie added a comment. Add debug info test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110044/new/ https://reviews.llvm.org/D110044 Files: clang/lib/AST/Type.cpp clang/test/AST/ast-dump-

[PATCH] D110044: Print nullptr_t namespace qualified within std::

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG131e8786640a: Print nullptr_t namespace qualified within std:: (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie added inline comments. Comment at: clang/lib/AST/Type.cpp:3045 case NullPtr: -return "nullptr_t"; +return "std::nullptr_t"; case Overload: aaron.ballman wrote: > dblaikie wrote: > > aaron.ballman wrote: > > > Should this be `::std::nullptr_

[PATCH] D110127: [Clang] Support typedef with btf_tag attributes

dblaikie added a comment. Given this is about being preserved into debug info - I imagine it'll have the same behavior as using a typedef in a function return type - whenever that currently shows up in the DWARF, this attribute would. Where it doesn't, this doesn't. So I wouldn't expect this f

[PATCH] D110201: [clang] Make -Rpass imply -Rpass=.*

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Great! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110201/new/ https://reviews.llvm.org/D110201 _

[PATCH] D110204: Make DiagnosticInfoResourceLimit's limit param required

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110204/new/ https://reviews.llvm.org/D110204

[PATCH] D109865: [NFC] `goto fail` has failed us in the past...

dblaikie added a comment. (A possibly more robust option - so that this doesn't regress if someone adds a new "return true" codepath & forgets to release the cleanup (it looks like a pretty long function with several exits, etc) - would be to move the code into an "impl" function and have the o

[PATCH] D109865: [NFC] `goto fail` has failed us in the past...

dblaikie added a comment. In D109865#3014106 , @beanz wrote: > I was talking with @lhames the other day about building a `doWithCleanup` > abstraction that could take a labmda to perform and a lambda to cleanup on > failure. > > I was thinking I may tak

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

dblaikie added a comment. There have been a few different patches going around related to mis-scoped things - imported entities and types, maybe other things too? (static variables? Or has that already been fixed now) Perhaps some summary of the state of all these patches sent to llvm-dev/linke

[PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

dblaikie created this revision. dblaikie added reviewers: probinson, aprantl. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This reverts c7f16ab3e3f27d944db72908c9c1b1b7366f5515 / r109694 - which suggested this was done to improv

[PATCH] D110673: [clang] Don't modify OptRemark if the argument is not relevant

dblaikie added a comment. It'd be good to understand/document (maybe document in the form of a test if possible) how downstream users are relying on this - perhaps it's not a valid reliance and we shouldn't maintain compatibility? Maybe it is and we should ensure some test coverage of the sort

[PATCH] D110665: [clang] Don't use the AST to display backend diagnostics

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:336-337 + for (auto &F : getModule()->functions()) { +if (const Decl *FD = Gen->GetDeclForMangledName(F.getName())) { + auto Loc = FD->getASTContext().getFullLoc(FD->getLocatio

[PATCH] D110898: Pass template parameters when printing template argument lists for function templates

dblaikie created this revision. dblaikie added a reviewer: rsmith. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Improve the application of D77598 by passing the template parameter list, allowin

[PATCH] D77598: Integral template argument suffix and cast printing

dblaikie added a comment. Came across this while trying to do "simplified template names" - producing template names in DWARF without template parameter lists as part of the textual name, then rebuilding that name from the structural representation of template parameters in DWARF (DW_TAG_templa

[PATCH] D110278: Adding doWithCleanup abstraction

dblaikie added inline comments. Comment at: clang/lib/Frontend/FrontendAction.cpp:553-565 + bool HasBegunSourceFile = false; + std::function Operation = [&]() { +return BeginSourceFileImpl(CI, RealInput, HasBegunSourceFile); + }; + std::function Cleanup = [&]() { +if

[PATCH] D110891: [inliner] Mandatory inlining decisions produce remarks

dblaikie added inline comments. Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:52 +namespace { +using namespace llvm::ore; mtrofin wrote: > wenlei wrote: > > mtrofin wrote: > > > wenlei wrote: > > > > curious why do we need anonymous namespace here? > > > iiuc

[PATCH] D77598: Integral template argument suffix and cast printing

dblaikie added a comment. In D77598#3035591 , @v.g.vassilev wrote: > In D77598#3035449 , @dblaikie wrote: > >> Came across this while trying to do "simplified template names" - producing >> template names in DWARF

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