[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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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. > 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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. 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] 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] 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] 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] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + hoy wrote: > dblaikie wrote: > > hoy wrote: > > > dblaikie wrote: > > > > aaron.ballman wrote: > > >

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

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + hoy wrote: > dblaikie wrote: > > aaron.ballman wrote: > > > dblaikie wrote: > > > > aaron.ballman wro

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

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + aaron.ballman wrote: > dblaikie wrote: > > aaron.ballman wrote: > > > dblaikie wrote: > > > > hoy wro

[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#2749625 , @shchenz wrote: > In D100630#2749594 , @dblaikie > wrote: > >> In D100630#2749550 , @shchenz >> wrote: >> >>> In D100630#2

[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#2749550 , @shchenz wrote: > In D100630#2748899 , @dblaikie > wrote: > >> Does this cause duplicate DW_TAG_reference_types in the output, if the input >> IR Has both DW_TAG_re

[PATCH] D101735: [WebAssembly] Reenable end-to-end test in wasm-eh.cpp

dblaikie added a comment. In D101735#2749716 , @aheejin wrote: > @dblaikie I can remove this one. This is not an important test anyway. But > where are we supposed to test the arguments clang driver invokes the backend > LLVM compilation with? This was

[PATCH] D101735: [WebAssembly] Reenable end-to-end test in wasm-eh.cpp

dblaikie added a comment. FWIW - please avoid end-to-end tests where possible (writing separate source to LLVM IR (in clang) and LLVM IR to assembly (in LLVM) tests). (recent discussions on the wasm simd instructions touched on this issue too) Repository: rG LLVM Github Monorepo CHANGES SIN

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

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + aaron.ballman wrote: > dblaikie wrote: > > hoy wrote: > > > dblaikie wrote: > > > > dblaikie wrote: >

[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. Does this cause duplicate DW_TAG_reference_types in the output, if the input IR Has both DW_TAG_reference_type and DW_TAG_rvalue_reference_types? Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:391 + // version. + dwarf::Tag FixedTag = (dwarf:

[PATCH] D102122: Support warn_unused_result on typedefs

dblaikie added a comment. In D102122#2748206 , @aaron.ballman wrote: > Let me start off by saying: thanks, I think this is really useful > functionality. As a ridiculously obvious example, Annex K has an integer type > alias `errno_t` and it would incr

[PATCH] D102122: Support warn_unused_result on typedefs

dblaikie updated this revision to Diff 343885. dblaikie added a comment. Herald added a subscriber: jdoerfert. Fixes for a few other test cases (though I wonder if these tests are overconstrained - do we need to be testing the list of things this attribute can be applied to in so many places?)

[PATCH] D102122: Support warn_unused_result on typedefs

dblaikie added a comment. Oh, one question: If we do move forward with this patch, how would we detect that the compiler supports this new use of warn_unused_result? (so that the feature detection macros in LLVM properly make the attribute a no-op unless we have a version of clang that supports

[PATCH] D102122: Support warn_unused_result on typedefs

dblaikie created this revision. dblaikie added a reviewer: aaron.ballman. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. While it's not as robust as using the attribute on enums/classes (the type information may be lost through a

[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. Mixed feelings - is this the only place where we're changing behavior for strict-DWARF in the frontend/clang? (maybe it'd be better to do it all in the same place, down in the backend when the IR is mapped to actual DWARF) @aprantl @probinson - you two have any thought

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

dblaikie added a subscriber: aaron.ballman. dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + hoy wrote: > dblaikie wrote: > > dblaikie wrote: > > > ho

[PATCH] D101766: [TableGen] [Clang] Clean up Options.td and add asserts

dblaikie added inline comments. Comment at: clang/include/clang/Driver/Options.td:1089 + LangOpts<"DoubleSquareBracketAttributes">, + Default, PosFlag, NegFlag, jansvoboda11 wrote: > Paul-C-Anagnostopoulos wrote: > > jansvoboda11 wrote: > > > Paul-C-Anagnost

[PATCH] D101805: [WebAssembly] Add codegen test for wasm_simd128.h

dblaikie added a comment. In D101805#2740150 , @tlively wrote: > @dblaikie, what's the best practice for making tests robust to this > difference? If you need a label or register (eg: if you want to check a branch branches to some particular spot) - th

[PATCH] D101805: [WebAssembly] Add codegen test for wasm_simd128.h

dblaikie added a comment. In D101805#2739814 , @tlively wrote: > Thanks, @hans! That's very surprising; I'll take a deeper look at what was > going wrong. Labels (& instructions/values) are only named in ASSERTS enabled builds (or you can opt into them

[PATCH] D101684: [WebAssembly] Add end-to-end codegen tests for wasm_simd128.h

dblaikie added a comment. In D101684#2737868 , @tlively wrote: > In D101684#2737842 , @penzn wrote: > >> I think there is another dimension to this aside from project composition - >> intrinsics have a tendency t

[PATCH] D101805: [WebAssembly] Add codegen test for wasm_simd128.h

dblaikie added a comment. In D101805#2737531 , @tlively wrote: > At just under 4 seconds on my machine, it's certainly not one of the quickest > clang lit tests, but neither is it one of the slowest. It looks like there > are many that take tens of seco

[PATCH] D101805: [WebAssembly] Add codegen test for wasm_simd128.h

dblaikie added a comment. how's the runtime of this test compared to others in clang? Is it worth splitting it up to improve test execution parallelism? (I'm guessing /probably not/ (guess while the file is long, it's relatively simple for the compiler compared to some other non-trivial templat

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

dblaikie added a comment. In D101566#2733172 , @aaronpuchert wrote: > In D101566#2730746 , @dblaikie > wrote: > >> Out of curiosity - have you tried it & measured any significant >> improvement/value in build t

[PATCH] D101766: [TableGen] [Clang] Clean up Options.td and add asserts

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

[PATCH] D101684: [WebAssembly] Add end-to-end codegen tests for wasm_simd128.h

dblaikie added a comment. I thought maybe /some/ of the other targets used end-to-end clang tests to test intrinsics, but I can't seem to find any (they seem to be a small minority, if there are any): grep -r -l intrin.h clang/test/ | xargs grep -L emit-llvm.*FileCheck | xargs grep RUN | les

[PATCH] D101684: [WebAssembly] Add end-to-end codegen tests for wasm_simd128.h

dblaikie added a comment. In D101684#2732522 , @aheejin wrote: > I think there's a clear upside on keeping this within clang/. > > 1. As @tlively said, there are many number of instructions to test and > keeping "C function - LLVM intrinsic" and "LLVM in

[PATCH] D101684: [WebAssembly] Add end-to-end codegen tests for wasm_simd128.h

dblaikie added a comment. In D101684#2732408 , @tlively wrote: > In D101684#2732395 , @dblaikie > wrote: > >> In D101684#2732366 , @tlively >> wrote: >> >>> In order to

[PATCH] D101684: [WebAssembly] Add end-to-end codegen tests for wasm_simd128.h

dblaikie added a comment. In D101684#2732366 , @tlively wrote: > In D101684#2732310 , @dblaikie > wrote: > >>> Assembly tests are generally discouraged in clang, but in this case we >>> actually care about the s

[PATCH] D101684: [WebAssembly] Add end-to-end codegen tests for wasm_simd128.h

dblaikie added a comment. > Assembly tests are generally discouraged in clang, but in this case we > actually care about the specific instruction being generated from the > intrinsics. I don't think this is a sound reason to add an end-to-end test in clang. The same is true of all clang tests,

[PATCH] D101671: Modules: Remove an extra early return, NFC

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

[PATCH] D101561: [Prototype] Introduce attribute for ignoring C++ ABI

dblaikie added a comment. In D101561#2730201 , @aeubanks wrote: > I'm first testing out if there are even noticeable benefits to doing this, > then if there are I'll put something together to post to cfe-dev. > https://crbug.com/1028731 for some backgro

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

dblaikie added a comment. In D100581#2730743 , @nickdesaulniers wrote: > Testing Diff 342071 on the mainline Linux kernel, just building x86_64 > defconfig triggers 19 instances of this warning; all look legit. ;) > > In D100581#2730708

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

dblaikie added a comment. In D101566#2730555 , @aaronpuchert wrote: > So I tried this in two code bases, both of which don't have `-Wweak-vtables` > enabled though. > > In the first (~500 TUs) there were a couple of interesting finds. But they > are ha

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

dblaikie added a comment. In D100581#2730557 , @nickdesaulniers wrote: > I see lots of instances from the kernel that look like this when reduced: > > $ cat foo.c > int page; > int put_page_testzero(int); > void foo (void) { > int zeroed; >

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

dblaikie added a comment. In D100581#2727432 , @mbenfield wrote: > In D100581#2727357 , @dblaikie > wrote: > >> > > > >> Got a link/examples of cases GCC does and doesn't warn about? I'd assume >> it'd have som

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

dblaikie added a comment. In D100581#2727330 , @mbenfield wrote: > Another try at these warnings, using the implementation strategy outlined by > rsmith. > > A couple other notes: > > - At the moment I've removed these warnings from the diagnostic groups

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

dblaikie added a comment. In D101566#2727244 , @aaronpuchert wrote: > In D101566#2726820 , @dblaikie > wrote: > >> Along time ago Clang had a fairly strong aversion to implementing "off by >> default" warnings

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

dblaikie added subscribers: respindola, doug.gregor. dblaikie added a comment. Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings (though clearly weak-vtables was an exception to that - it's a pretty esoteric warning even at the best of times/without this

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:2844 + auto *Fn = + dyn_cast(LV.getPointer(*this)->stripPointerCasts()); + if (DI && !Fn->getSubprogram()) Oh, please change this to cast, rather than dyn_cast before comm

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie accepted this revision. dblaikie added a comment. Looks good - thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100567/new/ https://reviews.llvm.org/D100567 ___ cfe-commits mailing list cfe

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:2843-2844 + CGDebugInfo *DI = CGM.getModuleDebugInfo(); + auto *Fn = dyn_cast(LV.getPointer(*this)); + if (DI && Fn && !Fn->getSubprogram()) +DI->EmitFunctionDecl(FD, FD->getLocation()

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. (ah, sorry, failed to submit comment) Comment at: clang/lib/CodeGen/CGExpr.cpp:2843-2844 + CGDebugInfo *DI = CGM.getModuleDebugInfo(); + auto *Fn = dyn_cast(LV.getPointer(*this)); + if (DI && Fn && !Fn->getSubprogram()) +DI->Emit

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

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + dblaikie wrote: > hoy wrote: > > dblaikie wrote: > > > hoy wrote: > > > > dblaikie wrote: > > > > > h

[PATCH] D100826: [Debug-Info][NFC] add -gstrict-dwarf support in backend

dblaikie accepted this revision. dblaikie added a comment. Sounds good - probably only need one test file, rather than two - can put a bunch of strict-dwarf related testing in that one file (& honestly, maybe don't even need to test every DWARF attribute gets the right handling if the handling

[PATCH] D100826: [Debug-Info][NFC] add -gstrict-dwarf support in backend

dblaikie added a comment. In D100826#2707145 , @shchenz wrote: > In D100826#2702135 , @dblaikie > wrote: > >> Do you know if you're going to be using LTO with DBX? If so, to respect this >> flag, it would need t

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

dblaikie added a comment. In D100581#2706554 , @xbolva00 wrote: > In D100581#2706467 , @dblaikie > wrote: > >> FWIW, I'd love it if we could do a full dead-store warning, which would be a >> superset of this. I

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

dblaikie added a comment. FWIW, I'd love it if we could do a full dead-store warning, which would be a superset of this. I think we have enough infrastructure in the analysis based warnings (I think the sufficiency of the infrastructure is demonstrated by the "may be used uninitialized" warning

[PATCH] D99299: Normalize interaction with boolean attributes

dblaikie added inline comments. Comment at: llvm/lib/IR/Attributes.cpp:660-663 +bool AttributeImpl::getValueAsBool() const { + assert(getValueAsString().empty() || getValueAsString() == "false" || getValueAsString() == "true"); + return getValueAsString() == "true"; +} ---

[PATCH] D100809: [Debug-Info] implement -gstrict-dwarf

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

[PATCH] D100826: [Debug-Info][NFC] add -gstrict-dwarf support in backend

dblaikie added a comment. Do you know if you're going to be using LTO with DBX? If so, to respect this flag, it would need to be added to LLVM IR module metadata (like the Dwarf Version and Debug Info Version fields) rather than as an MCOption. (we're pretty wishy-washy about what things we dec

[PATCH] D100809: [Debug-Info] implement -gstrict-dwarf

dblaikie added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:3549-3550 +Use DWARF extensions in later DWARF versions. + .. option:: -gz=, -gz (equivalent to -gz=zlib) This description is probably backwards/doesn't explicitly clarify whi

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. In D100567#2702394 , @yonghong-song wrote: > ping @dblaikie could you take a look of my new investigation? Yep, it's on my list to look at. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

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

dblaikie added a comment. In D99160#2698980 , @alok wrote: > In D99160#2671899 , @dblaikie wrote: > >> In D99160#2670460 , @djtodoro wrote: >> >>> In D99160#2669576

[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#2697728 , @shchenz wrote: > In D100630#2694681 , @probinson > wrote: > >> If DBX is going to be really pedantic about not recognizing tags or >> attributes that don't align w

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. In D100567#2697415 , @yonghong-song wrote: > To answer one of your questions above, if there is a function definition > before, dyn_cast(...) will return nullptr. I tried starting > from "Value" class and found dyn_cast to llv

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. In D100567#2696095 , @yonghong-song wrote: > Sorry, I know what is the segfault now after some debugging. It is because > `auto *Fn = dyn_cast(LV.getPointer(*this));` is a NULL > pointer after there is a definition before this

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

dblaikie added a comment. This usage doesn't seem to quite match the standard - which provides an existing instance of in_place_t for callers to use: std::optional o4(std::in_place, {'a', 'b', 'c'}); (to quote https://en.cppreference.com/w/cpp/utility/optional/optional ) Probably good to mat

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. Generally looks OK, but could you explain more about the problems with/motivation for the declaration/definition issue? (probably worth noting in the patch description maybe comment in code and test) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:2840 +// Emit debuginfo for the function declaration if the target wants to. +if (getContext().getTargetInfo().allowDebugInfoForExternalVar()) { + CGDebugInfo *DI = CGM.getModuleDebugInfo(); ---

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. ah, right, because this is powered by seeing the DeclRefExpr only in code that's codegen'd - fair enough. Thanks for checking! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100567/new/ https://reviews.llvm.org/D100567 __

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

dblaikie added a comment. What happens for this program: extern void f1(); void f2(void *); inline void f3() { f2(f1); } ... Even when `f3` is never called, I'm guessing your change will cause `f1` to be emitted? Also something like this: void f1(); int main() { int x =

[PATCH] D100378: [AST] Use IntrusiveRefCntPtr for Introspection LocationCall.

dblaikie added inline comments. Comment at: clang/lib/Tooling/NodeIntrospection.cpp:60 + if (LHS.first == RHS.first) +return LHS.second->name() < RHS.second->name(); + return LHS.first < RHS.first; njames93 wrote: > dblaikie wrote: > > njames93 wrote: > > >

[PATCH] D100378: [AST] Use IntrusiveRefCntPtr for Introspection LocationCall.

dblaikie added inline comments. Comment at: clang/lib/Tooling/NodeIntrospection.cpp:60 + if (LHS.first == RHS.first) +return LHS.second->name() < RHS.second->name(); + return LHS.first < RHS.first; njames93 wrote: > dblaikie wrote: > > njames93 wrote: > > >

[PATCH] D100378: [AST] Use IntrusiveRefCntPtr for Introspection LocationCall.

dblaikie added inline comments. Comment at: clang/lib/Tooling/NodeIntrospection.cpp:60 + if (LHS.first == RHS.first) +return LHS.second->name() < RHS.second->name(); + return LHS.first < RHS.first; njames93 wrote: > This is a slight change in behaviour, The

[PATCH] D93185: [docs] Use make_unique in FrontendAction example

dblaikie added subscribers: MaskRay, dblaikie. dblaikie added a comment. @thakis - just FYI, Phab doesn't send mail to the mailing list when a patch is approved without any text. Please include some text when approving patches to make sure there's a record on the mailing list. (@maskray has a br

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

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39 +static int go(a) int a; +{ + return glob + a; +} + + hoy wrote: > dblaikie wrote: > > hoy wrote: > > > dblaikie wrote: > > > > hoy wrote: > > > > > dblaik

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

dblaikie added a comment. In D99160#2670460 , @djtodoro wrote: > In D99160#2669576 , @dblaikie wrote: > >> In D99160#2668977 , @djtodoro wrote: >> >>> I think that the Debug

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

dblaikie added a comment. In D99160#2668977 , @djtodoro wrote: > I think that the Debug Entry Values feature should not be enabled by default > for non optimized code, so the `TargetOptions::ShouldEmitDebugEntryValues()` > should be patched with checking

[PATCH] D99238: [DebugInfo] Enable the call site parameter feature by default

dblaikie added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1648 - if (Opts.OptimizationLevel > 0 && Opts.hasReducedDebugInfo() && + if (Opts.hasReducedDebugInfo() && llvm::is_contained(DebugEntryValueArchs, T.getArch())) alok

[PATCH] D99238: [DebugInfo] Enable the call site parameter feature by default

dblaikie added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1648 - if (Opts.OptimizationLevel > 0 && Opts.hasReducedDebugInfo() && + if (Opts.hasReducedDebugInfo() && llvm::is_contained(DebugEntryValueArchs, T.getArch())) prob

[PATCH] D99703: [debug-info][XCOFF] set `-gno-column-info` by default for DBX

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good to me Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3974 + // Microsoft debuggers don't handle missing end columns well, and the AIX + // debugger DBX a

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

dblaikie added subscribers: akhuang, probinson, rnk. dblaikie added a comment. @rnk @akhuang - I think we touched on this before maybe in some other thread/patch/discussion? Ah, @probinson mentioned in the linked thread: > Is this https://bugs.llvm.org/show_bug.cgi?id=44170 which had a tentativ

[PATCH] D98146: OpaquePtr: Turn inalloca into a type attribute

dblaikie added a comment. In D98146#2654598 , @arsenm wrote: > 4fefed65637ec46c8c2edad6b07b5569ac61e9e5 > Please include the "Differential Revision: ..." line in the commit message - i

<    5   6   7   8   9   10   11   12   13   14   >