Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (432 w, 1 d)

Recent Activity

Today

dblaikie accepted D94739: ADT: Fix reference invalidation in SmallVector::emplace_back and the size+value version of SmallVector::assign.

Haven't reviewed the logic in detail - but generally looks about right to me.

Tue, Jan 19, 5:26 PM · Restricted Project
dblaikie accepted D95001: [CodeView] Emit function types in -gline-tables-only..

Looks good!

Tue, Jan 19, 5:11 PM · Restricted Project
dblaikie added inline comments to D95001: [CodeView] Emit function types in -gline-tables-only..
Tue, Jan 19, 4:56 PM · Restricted Project
dblaikie added a comment to D94347: [WebAssembly] locals can now be indirect in DWARF.

(haven't followed this whole conversation - but I will say that DW_AT_calling_convention is used/needed for knowing how to call a function to meet the ABI (do you stuff the member variables in certain registers then call, or do you make a copy on the stack and stuff a pointer to that copy in a register then call) - the DW_AT_locations of variables aren't used for calling (they may not be present - perhaps there's a member function you have a declaration for - or even a function you have no DWARF for, but you can demangle it and find the parameter types in the DWARF) as they may not. describe the location of variables at the start of the function/as-the-ABI-specifies them. so probably best not to use that feature as a property for determining DW_AT_locations of variables)

Tue, Jan 19, 4:02 PM · Restricted Project
dblaikie added a comment to D94976: [DWARF] Create subprogram's DIE in the unit specified by its DISubprogram.

(seems there is perhaps some nearby pretty problematic issues I'm just writing down here because I saw it: Generally there are two kinds of DISubprograms: definition DISubprograms are "distinct" and referenced/kept alive from an llvm::Function's subprogram attachment. Declaration DISubprograms are not distinct and referenced from the member list of a DICompositeType, for instance. That's the simple case - if IR Linking occurs, one copy of an llvm::Function definition is retained, along with its referenced DISubprogram, simple enough. But it seems we are keeping definition DISubprograms alive through a few other means (or declaration DISubprograms, perhaps) - such as from call site metadata, using declarations, and the scope chains for types like lambdas. These DISubprograms aren't properly deduplicated because they can't necessarily piggyback on the llvm::Function deduplication: They are referenced from multiple parts of the debug info metadata - this can lead to buggy/weird DWARF like this:

Tue, Jan 19, 3:27 PM · Restricted Project
dblaikie added inline comments to D95001: [CodeView] Emit function types in -gline-tables-only..
Tue, Jan 19, 2:50 PM · Restricted Project
dblaikie added inline comments to D94555: [LLD][COFF] Avoid std::vector resizes during type merging.
Tue, Jan 19, 1:49 PM · Restricted Project
dblaikie added a comment to D94882: [MC] Upgrade DWARF version to 5 upon .file 0.

A few thoughts:

Tue, Jan 19, 12:08 PM · Restricted Project
dblaikie added inline comments to D94555: [LLD][COFF] Avoid std::vector resizes during type merging.
Tue, Jan 19, 11:09 AM · Restricted Project
dblaikie added inline comments to D94555: [LLD][COFF] Avoid std::vector resizes during type merging.
Tue, Jan 19, 10:45 AM · Restricted Project

Yesterday

dblaikie added a comment to D94882: [MC] Upgrade DWARF version to 5 upon .file 0.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

Mon, Jan 18, 5:28 PM · Restricted Project
dblaikie added a comment to D94888: [lldb] Add -Wl,-rpath to make tests run with fresh built libc++.

(Works for me, FWIW - takes my local run down from 42 failures in check-lldb-api to 1 (a timeout))

Mon, Jan 18, 5:22 PM · Restricted Project

Sun, Jan 17

dblaikie accepted D93167: [NFC] Add CMakeUserPresets.json filename to .gitignore.

Sounds good, thanks!

Sun, Jan 17, 10:51 AM · Restricted Project
dblaikie accepted D94858: [STLExtras] Add a default value to drop_begin.

Sounds good, thanks!

Sun, Jan 17, 10:50 AM · Restricted Project

Fri, Jan 15

dblaikie updated subscribers of D94846: Allow breakpoints to be set on C++11 inline initializers.

"But because their source lines are outside the function source range"

Fri, Jan 15, 5:10 PM · Restricted Project
dblaikie added inline comments to D94822: [TableGen] Improve algorithm for inheriting class template arguments and fields.
Fri, Jan 15, 4:36 PM · Restricted Project
dblaikie added inline comments to D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location.
Fri, Jan 15, 4:33 PM · Restricted Project
dblaikie accepted D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location.

Couple of optional pieces.

Fri, Jan 15, 2:39 PM · Restricted Project
dblaikie committed rG76f5c5a7b059: [ADT][Support] Fix C4146 error from MSVC (authored by vinograd47).
[ADT][Support] Fix C4146 error from MSVC
Fri, Jan 15, 2:34 PM
dblaikie closed D94417: [ADT][Support] Fix C4146 error from MSVC.
Fri, Jan 15, 2:34 PM · Restricted Project

Thu, Jan 14

dblaikie added a comment to D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location.

Ah, seems I was confused by the patch title - it looks like these values aren't used for FlagFwdDecl records - forward declarations bail out 3356, not using the file/line created on 3338/3339.

Seems like this change might lead to a situation of files but no line numbers, which seems a bit strange to me - well, at least for __va_list_tag, which isn't anymore associated with the file than with the line. Perhaps both should be addressed?

__va_list_tag is synthesized by Clang CreateX86_64ABIBuiltinVaListDecl for __builtin_va_arg. I think giving it the filename of CurLoc is fine. Giving it the line of the CurLoc is arbitrary. CurLoc may be from a very unrelated thing.

Emm.. The title was wrong. I did not read 3319 if (T && !T->isForwardDecl()) carefully...

Thu, Jan 14, 9:33 PM · Restricted Project
dblaikie added a comment to D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location.

Ah, seems I was confused by the patch title - it looks like these values aren't used for FlagFwdDecl records - forward declarations bail out 3356, not using the file/line created on 3338/3339.

Thu, Jan 14, 8:47 PM · Restricted Project
dblaikie added a comment to D94542: [X86] Default to -x86-pad-for-align=false to drop assembler difference with or w/o -g.

I'm not convinced that disabling these optimizations is warranted. I'm not actively opposing the patch, just want to list my concerns for the record.

As a general matter, -O3 -g means "perform optimization while preserving the best debug experience we can", not "strictly w/o reducing debug quality optimize as best as possible". We have numerous places in the optimizer that we perform optimizations that destroy debug information. We also have lots of places - one use restrictions which haven't been audited for instance - where the presence of debug info restricts optimization. We treat the later as bugs, not the former.

It's not clear to be me why this particular optimization should be disabled just because it happens to emit different code w/ and w/o debuging enabled.

The bug (https://bugs.llvm.org/show_bug.cgi?id=42138) that triggered this review appears to be an automated testing framework. I fully understand the value of that type of automatic bug discovery, but when the framework reports something which is not a bug, the framework should be fixed. I believe this is a bug in the reporting framework, not the assembler.

Thu, Jan 14, 8:16 PM · Restricted Project
dblaikie added a comment to D92136: [clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows.

Bit out of my wheelhouse hopefully @rsmith or @rnk could chime in at some point.

Thu, Jan 14, 7:33 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D94735: CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location.
Thu, Jan 14, 6:20 PM · Restricted Project
dblaikie accepted D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

Looks worth another go

Thu, Jan 14, 3:02 PM · Restricted Project
dblaikie added a comment to D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.

Thanks for the pointers!

Thu, Jan 14, 1:39 PM · Restricted Project
dblaikie added inline comments to D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=.
Thu, Jan 14, 1:02 PM · Restricted Project
dblaikie added a comment to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

I just triaged the gcc 5.4 build break. It appears that std::is_trivially_copy_constructible<T> instantiates the copy constructor (which is... strange). So we can't query a compile-time property without the compiler forcing instantiation of the deleted copy constructor in this case.

This bug exists all the way up through gcc 7.3. Here's a godbolt link which reproduces the problem:
https://godbolt.org/z/ETW7f1

If we use llvm::is_trivially_copy_constructible<T> instead of std::is_trivially_copy_constructible<T>, then everything appears to be fine.

Interestingly, std::is_trivially_copy_assignable<T> appears not to instantiate the copy assignment because the compiler isn't complaining about its use. So I'm changing the OptionalStorage specialization condition to:

template <typename T, bool = (is_trivially_copy_constructible<T>::value &&
                              std::is_trivially_copy_assignable<T>::value &&
                              (std::is_trivially_move_constructible<T>::value ||
                               !std::is_move_constructible<T>::value) &&
                              (std::is_trivially_move_assignable<T>::value ||
                               !std::is_move_assignable<T>::value))>
Thu, Jan 14, 12:49 PM · Restricted Project
dblaikie accepted D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input.

Is there any way to condition this on the type of the output, rather than the input? (or, more specifically, on whether machine code is being generated)

Or maybe we could always pass the split-dwarf-file down through LLVM and not need to conditionalize it at all? It'd be a no-op if there's no DWARF in the IR anyway?

I tried replacing if (IRInput || Args.hasArg(options::OPT_g_Group)) { with if (1), -gsplit-dwarf may produce .dwo for regular non-g .c compile.

Are you saying that if you make that change -gsplit-dwarf does cause .dwo files to be created for non-g .c compiles? Do the dwo files have anything in them? I had modified llvm to dynamically choose split or non-split based on whether there was enough data to be worth splitting into a .dwo file, but I guess that situation might still be producing an empty .dwo file which isn't ideal - I haven't tested that.

% clang a.c -gsplit-dwarf -c
% readelf -WS a.dwo                
There are 2 section headers, starting at offset 0x50:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .strtab           STRTAB          0000000000000000 000040 000009 00      0   0  1
Thu, Jan 14, 11:24 AM · Restricted Project
dblaikie requested changes to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Thu, Jan 14, 10:06 AM · Restricted Project
dblaikie reopened D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

Taking a look at the gcc 5.4 failure now.

Now that the patch has been committed & reverted, do I start a new patch when fixes for identified issues are incorporated?

Thu, Jan 14, 10:06 AM · Restricted Project
dblaikie added a comment to D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input.

Is there any way to condition this on the type of the output, rather than the input? (or, more specifically, on whether machine code is being generated)

Or maybe we could always pass the split-dwarf-file down through LLVM and not need to conditionalize it at all? It'd be a no-op if there's no DWARF in the IR anyway?

I tried replacing if (IRInput || Args.hasArg(options::OPT_g_Group)) { with if (1), -gsplit-dwarf may produce .dwo for regular non-g .c compile.

Thu, Jan 14, 9:48 AM · Restricted Project
dblaikie added a comment to D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert.

Why do we even want to bend over backwards and try to handle broken code correctly,
at the cost of regressed performance for everything,
instead of just adding defensive asserts against such problems,
and letting/telling the user fix the code?

I believe the standard library supports this, so I think it's a somewhat awkward limitation to have a standard-library-like container that misses that. (of course the small-mode isn't entirely compatible with std::vector, etc anyway - so some divergence is possible)

But currently the cost (compile time) is too high to be acceptable.

Thu, Jan 14, 9:40 AM · Restricted Project
dblaikie added a comment to D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert.

Why do we even want to bend over backwards and try to handle broken code correctly,
at the cost of regressed performance for everything,
instead of just adding defensive asserts against such problems,
and letting/telling the user fix the code?

Thu, Jan 14, 9:02 AM · Restricted Project
dblaikie added a comment to D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input.

Is there any way to condition this on the type of the output, rather than the input? (or, more specifically, on whether machine code is being generated)

Thu, Jan 14, 9:01 AM · Restricted Project

Wed, Jan 13

dblaikie added inline comments to D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=.
Wed, Jan 13, 10:45 PM · Restricted Project
dblaikie added inline comments to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Wed, Jan 13, 8:51 PM · Restricted Project
dblaikie accepted D94649: [StringExtras] Rename SubsequentDelim to ListSeparator.

Sounds good to me - but give it a day or two. @MaskRay or others might want to weigh in.

Wed, Jan 13, 8:49 PM · Restricted Project
dblaikie added a comment to D93781: ADT: Fix reference invalidation in SmallVector::resize.

Looks good (did you happen to try this with sanitizers without the fix applied & observe a sanitizer failure when running the updated test? Would be nice to know that's actually working as intended)

No, but just now I ran with the SmallVector.h part of this patch mostly reverted (didn't add back the assertion), and the test fails:

/Users/dexonsmith/data/github/llvm-project/working/llvm/unittests/ADT/SmallVectorTest.cpp:1139: Failure
      Expected: N
      Which is: 3
To be equal to: V.back()
      Which is: 8-byte object <01-00 00-00 00-00 00-00>
/Users/dexonsmith/data/github/llvm-project/working/llvm/unittests/ADT/SmallVectorTest.cpp:1145: Failure
      Expected: 1
To be equal to: V.back()
      Which is: 8-byte object <01-04 00-00 00-00 00-00>

The line numbers are different vs. the patch posted here because of a rebase, but this seems to confirm the test is working even without sanitizers.

About to commit (check-llvm just finished on the rebase); I can follow up with ASan post-commit if you'd like (let me know if you would).

Wed, Jan 13, 8:48 PM · Restricted Project
dblaikie accepted D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=.

Looks alright. I wouldn't mind knowing a bit more about how these things interact if different types of input files are listed together in a single command line, though.

Wed, Jan 13, 8:38 PM · Restricted Project
dblaikie added a comment to D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=.

In implicit thinlto this seemed to work for me without any changes:

$ clang++-tot -flto=thin test.cpp -g -c
$ clang++-tot -flto=thin -fuse-ld=lld -gsplit-dwarf test.o && llvm-dwarfdump-tot a.out
a.out:  file format elf64-x86-64
Wed, Jan 13, 7:52 PM · Restricted Project
dblaikie added a comment to D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert.

Reverted in 56d1ffb927d03958a7a31442596df749264a7792 due to an error on Windows:
http://lab.llvm.org:8011/#/builders/127/builds/4489/steps/7/logs/stdio
if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.

Guess maybe something in the std::is_same<std::remove_const_t<std::remove_reference_t<ArgType>>, T>::value isn't working quite right on MSVC? (could test that with godbolt, maybe) - is it needed? Since it's on both insert_one_maybe_copy and there's an implementation detail/doesn't need to protect from violations of that constraint (admittedly I'm proposing removing it because maybe MSVC is violating it... )? And instead SFINAE/enable_if only on the TakesParamByValue part?

Thanks for looking; I think you're right. The constraint was a trick to allow use of SFINAE since TakesParamByValue didn't depend on the template parameter so I can't just remove it, I need a different solution...

...but I've found one, and it's a bit cleaner too. Instead of the indirection through insert_one_maybe_copy, I've added a forwarding function defined in the POD vs. non-POD base classes called forward_value_param.

I'll commit the new version once I confirm check-llvm passes locally.

Wed, Jan 13, 7:47 PM · Restricted Project
dblaikie added a comment to D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert.

Reverted in 56d1ffb927d03958a7a31442596df749264a7792 due to an error on Windows:
http://lab.llvm.org:8011/#/builders/127/builds/4489/steps/7/logs/stdio
if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.

Wed, Jan 13, 7:10 PM · Restricted Project
dblaikie added a comment to D80391: [Driver] Don't make -gsplit-dwarf imply -g2.

From @dblaikie's email reply:

^ this I would consider to be a bug. -g should be needed to generate debug info into the IR, and -gsplit-dwarf should be needed to choose how debug info already in the IR should be placed into object files or dwo files.

Usually (non-LTO) these two steps are in the same compile, so -g and -gsplit-dwarf go together. But for LTO cases, I think it's suitable for -g to be needed for IR generation, and -gsplit-dwarf to be needed for object code generation, without needing -g as well in that latter step (the -g was already specified at IR generation time, and that shoul be enough).

We can see that behavior I'm describing as desirable in implicit ThinLTO I showed earlier in the thread.

I suggest this should be the desired behavior, because -g generally has no effect on IR->object code generation, so it seems strange/unnecessary to me to gate -gsplit-dwarf behind -g in that case. The -g was already specified and used during IR generation (& the -gN level, -gmlt behavior-etc has all been handled in IR generation (or embedded in the IR for use later) - so to me, specifying -g again during object code generation would be/is strange, because any special values would be ignored (-g1/g2/g3/etc would not be respected - the N value would only be respected when passed to the IR generation step))

Got it. -fthinlto-index= (thinlto backend compile) operates on bitcode files and does not IR generation. It isn't suitable for -g to be needed to use.
-gsplit-dwarf is both an IR generation option and an object file generation option. It is needed for thinlto backend compiles.

Wed, Jan 13, 6:46 PM · Restricted Project
dblaikie added a comment to D80391: [Driver] Don't make -gsplit-dwarf imply -g2.

For Chrome on Chrome OS, this is https://crbug.com/1158215

Here, we saw our links fail with "output file too large". Investigation revealed that debug info was being included in the binary, instead of in .dwo files as expected. That turned out to be caused by this change.

It seems that there are cases where -gsplit-dwarf is passed, but it does not take effect because there isn't also a -g (or -g<level>) option:

clang -c -fthinlto-index=foo.o.thinlto.bc -gsplit-dwarf foo.o -o foo.bin.o

Shows no .dwo file and debug info in foo.bin.o, whereas

clang -c -fthinlto-index=foo.o.thinlto.bc -gsplit-dwarf -g foo.o -o foo.bin.o

shows a .dwo file which is referenced from foo.bin.o

The only thing that is different here is that the latter command has "-g" whereas the former doesn't.

In other words, the new implementation still doesn't make -g and -gsplit-dwarf fully orthogonal; instead of -gsplit-dwarf always turning on debug fission, it *only* turns it on when there is also a -g or -g<level> option - at least in distributed ThinLTO code generation.

It seems that orthogonality was the motivation for this change, and that -gsplit-dwarf without -g is supposed to work in the linker (per MaskRay's comment about -g being ignored by the linker).

Can we make it so that -gsplit-dwarf turns on debug fission and we then use that even if no -g options were passed?

Wed, Jan 13, 6:03 PM · Restricted Project
dblaikie added a comment to D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

Wed, Jan 13, 6:01 PM · Restricted Project, Restricted Project
dblaikie added a comment to D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

This was kind of motivated by the issue in https://bugs.llvm.org/show_bug.cgi?id=48432, even though this patch doesn't solve that particular bug. I guess the general issue is that in CV the only way to make the functions unique is by their display name or type. So previously we were going in the direction of differentiating functions by the display name (like using the qualified name), but it seems like it's more size-efficient to use types instead, which is what this patch does.

Wed, Jan 13, 4:27 PM · Restricted Project, Restricted Project
dblaikie added a comment to D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types.

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

Wed, Jan 13, 3:31 PM · Restricted Project, Restricted Project
dblaikie committed rG854f0984f0b7: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable (authored by jplayer-nv).
Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable
Wed, Jan 13, 3:24 PM
dblaikie closed D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Wed, Jan 13, 3:24 PM · Restricted Project
dblaikie accepted D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert.

Looks good, thanks!

Wed, Jan 13, 1:24 PM · Restricted Project
dblaikie added inline comments to D80391: [Driver] Don't make -gsplit-dwarf imply -g2.
Wed, Jan 13, 1:16 PM · Restricted Project
dblaikie added inline comments to D80391: [Driver] Don't make -gsplit-dwarf imply -g2.
Wed, Jan 13, 1:05 PM · Restricted Project
dblaikie added a comment to D94417: [ADT][Support] Fix C4146 error from MSVC.

Can you commit this yourself (do you have commit access), or do you need someone to commit it for you?

Wed, Jan 13, 1:02 PM · Restricted Project
dblaikie accepted D93749: [llvm-dwp] Add a command line option to set the target triple.

Code change looks good - test might be able to be tweaked a bit before committing.

Wed, Jan 13, 1:02 PM · Restricted Project
dblaikie accepted D94237: [clang] Use SourceLocations in unions [NFCI].

Sounds good

Wed, Jan 13, 12:57 PM · Restricted Project

Tue, Jan 12

dblaikie added a comment to D94391: CGDebugInfo: Drop Loc.isInvalid() special case from getLineNumber.

I'll leave this one to @aprantl

Tue, Jan 12, 7:07 PM · Restricted Project
dblaikie accepted D83305: [ADT] Fix join_impl using the wrong size when calculating total length.

Sounds good - thanks!

Tue, Jan 12, 7:07 PM · Restricted Project
dblaikie committed rG0d88d7d82bc4: Delete unused function (was breaking the -Werror build) (authored by dblaikie).
Delete unused function (was breaking the -Werror build)
Tue, Jan 12, 3:30 PM
dblaikie added a comment to D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.

Do you have python enabled in the build? (https://lldb.llvm.org/resources/build.html#preliminaries)

The cmake option LLDB_ENABLE_PYTHON defaults to Auto which has tripped me up in the past. If you set it to YES then you'll get a build error if it fails to find a suitable Python instead of silently disabling it.

Tue, Jan 12, 3:29 PM · Restricted Project
dblaikie added a comment to D94391: CGDebugInfo: Drop Loc.isInvalid() special case from getLineNumber.

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

Tue, Jan 12, 3:09 PM · Restricted Project
dblaikie accepted D94381: [test] Add Clang side tests for -fdebug-info-for-profiling.

Sounds good

Tue, Jan 12, 12:34 PM · Restricted Project
dblaikie added inline comments to D94381: [test] Add Clang side tests for -fdebug-info-for-profiling.
Tue, Jan 12, 12:17 PM · Restricted Project
dblaikie updated subscribers of D94391: CGDebugInfo: Drop Loc.isInvalid() special case from getLineNumber.

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

Tue, Jan 12, 12:10 PM · Restricted Project
dblaikie accepted D94455: Add sample-profile-suffix-elision-policy attribute with -funique-internal-linkage-names..

Sounds OK

Tue, Jan 12, 12:07 PM · Restricted Project
dblaikie accepted D94417: [ADT][Support] Fix C4146 error from MSVC.

Seems OK - I don't know if there's a nicer way to handle the latter cases that need the 1 + ~N.

Tue, Jan 12, 11:26 AM · Restricted Project
dblaikie added a comment to D94237: [clang] Use SourceLocations in unions [NFCI].

Do you need to run the destructor before placement new in these situations?

Tue, Jan 12, 11:25 AM · Restricted Project
dblaikie added a comment to D94494: canCreateUndefOrPoison: dyn_cast -> dyn_cast_or_null.

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

Probably just in its own file (we don't do a very good job of keeping a lot of related tests in a single file together - usually just add more files, it's not ideal but often hard to find other good groupings) - but if you're looking for related test files, try breaking the pass in some obvious way

Tue, Jan 12, 10:43 AM · Restricted Project
dblaikie added a comment to D94494: canCreateUndefOrPoison: dyn_cast -> dyn_cast_or_null.

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

Tue, Jan 12, 10:43 AM · Restricted Project
dblaikie added a comment to D94377: [StringExtras] Add a helper class for comma-separated lists.

The name SubsequentDelim looks a bit strange to me. Maybe others have suggestions on the name?
(personally I might choose AddDelimUnlessFirst)

Tue, Jan 12, 8:46 AM · Restricted Project

Mon, Jan 11

dblaikie added a comment to rG6215c1b778f6: CGDebugInfo: Delete redundant test.

The called completeUnusedClass is immediately below.

DebugKind <= codegenoptions::DebugLineTablesOnly is a live code path.

Mon, Jan 11, 1:12 PM
dblaikie added a comment to D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.

I think for full generality the standard library does some things to check if a type can be derived from before using it in this way - but we probably don't need all that machinery for LLVM-internal allocator stuff like this?

We just have to make sure people don't use an allocator that's defined with the final keyword. I doubt we are going to run into many cases of that.
I have done the template magic to get it make it work if someone wants to use an allocator marked final, should I include it?

Mon, Jan 11, 12:53 PM · Restricted Project
dblaikie added a comment to rG6215c1b778f6: CGDebugInfo: Delete redundant test.

I think this is here for consistency with the other complete* functions - perhaps it could be left as an assertion?

Mon, Jan 11, 12:47 PM
dblaikie added a comment to rGb8d28420885a: CGDebugInfo: Delete unneeded UnwrapTypeForDebugInfo.

Might still be a useful conversion for possible future callsites? Or are all the call sites pretty clearly not using this?

Mon, Jan 11, 12:47 PM
dblaikie added a comment to D94391: CGDebugInfo: Drop Loc.isInvalid() special case from getLineNumber.

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Mon, Jan 11, 12:46 PM · Restricted Project
dblaikie added a comment to D94417: [ADT][Support] Fix C4146 error from MSVC.

Perhaps we could rewrite the code to use ~ or the like instead of unary - if that works?

Mon, Jan 11, 12:12 PM · Restricted Project
dblaikie accepted D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.

I think for full generality the standard library does some things to check if a type can be derived from before using it in this way - but we probably don't need all that machinery for LLVM-internal allocator stuff like this?

Mon, Jan 11, 12:10 PM · Restricted Project
dblaikie accepted D94440: [ADT] Add makeIntrusiveRefCnt helper function.

Looks good - thanks!

Mon, Jan 11, 12:03 PM · Restricted Project

Sun, Jan 10

dblaikie updated subscribers of D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.

Thanks for all the context - so sounds like mostly based on (3) the
recommendation would be for this to be an API test (is there a way to test
the line table directly? good place for reference on the SB API options - I
looked at a few tests and they seemed quite different
( lldb/test/API/functionalities/breakpoint/move_nearest/TestMoveNearest.py
and lldb/test/API/commands/breakpoint/set/func-regex/TestBreakpointRegexError.py
) in the way they're written, so not sure what the norms are/how they work).

Sun, Jan 10, 5:37 PM · Restricted Project
dblaikie accepted D94377: [StringExtras] Add a helper class for comma-separated lists.

Sure, sounds alright.

Sun, Jan 10, 2:18 PM · Restricted Project

Sat, Jan 9

dblaikie added inline comments to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.
Sat, Jan 9, 10:55 AM · Restricted Project, debug-info

Fri, Jan 8

dblaikie committed rG33c8e16f660d: PR47391: Canonicalize DIFiles (authored by umesh.kalappa0).
PR47391: Canonicalize DIFiles
Fri, Jan 8, 10:12 PM
dblaikie closed D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.
Fri, Jan 8, 10:11 PM · Restricted Project, debug-info
dblaikie added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

Here is a suggestion for a plan forward. Let's get these patches along with D94154 in. No correctness issues but a missed opportunity. I will work with @rnk and @dblaikie and send out a patch where I move the uniqueification to clang? That patch will also do linkage name for C functions with mangled name when uniqueification is needed. Does that sound reasonable? As for timeline, I can do this in two weeks.

Fri, Jan 8, 10:03 PM · Restricted Project, Restricted Project
dblaikie accepted D93747: Rename debug linkage name with -funique-internal-linkage-names.

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

Fri, Jan 8, 9:40 PM · Restricted Project, Restricted Project
dblaikie accepted D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

Looks good - thanks!

Fri, Jan 8, 9:36 PM · Restricted Project
dblaikie added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
In D94154#2487745, @hoy wrote:
In D94154#2486081, @hoy wrote:
In D94154#2482425, @rnk wrote:

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

I'm pretty sure most demangling tools such as c++filt check for a leading prefix of _+Z before demangling, so I don't think there are any concerns for C names. A reasonable demangler would pass them through as is.

Yep - but passing them through unmodified was causing problems for gdb which was demangling the mangled names as they appear in the DWARF and using the unmangled name to figure out how to do name lookup. So if the symbol wasn't getting unmangled you wouldn't be able to "break f1" instead you'd have to "break f1.__part.1" which would be an unfortunate regression in debuggability.

But it seems like that only applies if a mangled name is present in the DWARF at all - if no mangled name is present, and the debug info just gives the pretty name it works OK. Bit weird to have no record of the real symbol name in the DWARF, but so far it doesn't seem to cause any problems? So I'm OK-ish with this.

Do you have plans to fix this more generally? (I think to fix it more generally, you might need to move this feature up to clang and have clang mangle the name then add the suffix (at least for itanium mangling - not sure if windows mangling supports arbitrary suffixes like this, @rnk might know) - that way for C functions you can force/enable the mangling as is done for attribute((overloadable)) and others (I think attribute((enable_if)) also causes mangling of C functions))

That was Sriram's original idea: have the mangler do it. I reviewed the code. Modifying the mangler was fairly complicated, so I suggested doing it in a pass. The original pass also operated by appending a suffix after mangling, it just happened earlier. There isn't a good way to "embed" this uniquification into the Itanium mangling scheme, so far as I am aware.

Oh, I was still in favor of adding it after the mangling (the current "mangled.__part.number") but I thought it may be necessary to force mangling on C functions before adding the suffix, if they needed to be demangleable for debug info purposes.

That all said, I don't see any reason to block this decimilization change. It's limited in scope and an obvious improvement.

My concern was that it was layering more workarounds on a patch series that might be going in the wrong direction overall.

Anyway, sounds like, if I'm understanding/tested correctly, that the issue with unmangleability isn't about the symbol name itself but the DW_AT_linkage_name in the DWARF, so it seems like if that isn't present then there's no issue if the real symbol name can't be unmangled/back to the simple name. (but there's some reason the DW_AT_linkage_name, if present, must match the symbol name? (so it can't be the original unsuffixed mangled name))

One reason that DW_AT_linkage_name should be consistent with the real linkage name is to favor AutoFDO. DW_AT_linkage_name is used to generate profiles for inlined functions.

How's this work for C functions? Same sort of problem?

Yes, same problem exists for C functions. For C functions without DW_AT_linkage_name, DW_AT_name will be used by AutoFDO and that's not uniquefied.

Fri, Jan 8, 1:30 PM · Restricted Project, Restricted Project
dblaikie added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
In D94154#2486081, @hoy wrote:
In D94154#2482425, @rnk wrote:

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

I'm pretty sure most demangling tools such as c++filt check for a leading prefix of _+Z before demangling, so I don't think there are any concerns for C names. A reasonable demangler would pass them through as is.

Yep - but passing them through unmodified was causing problems for gdb which was demangling the mangled names as they appear in the DWARF and using the unmangled name to figure out how to do name lookup. So if the symbol wasn't getting unmangled you wouldn't be able to "break f1" instead you'd have to "break f1.__part.1" which would be an unfortunate regression in debuggability.

But it seems like that only applies if a mangled name is present in the DWARF at all - if no mangled name is present, and the debug info just gives the pretty name it works OK. Bit weird to have no record of the real symbol name in the DWARF, but so far it doesn't seem to cause any problems? So I'm OK-ish with this.

Do you have plans to fix this more generally? (I think to fix it more generally, you might need to move this feature up to clang and have clang mangle the name then add the suffix (at least for itanium mangling - not sure if windows mangling supports arbitrary suffixes like this, @rnk might know) - that way for C functions you can force/enable the mangling as is done for attribute((overloadable)) and others (I think attribute((enable_if)) also causes mangling of C functions))

That was Sriram's original idea: have the mangler do it. I reviewed the code. Modifying the mangler was fairly complicated, so I suggested doing it in a pass. The original pass also operated by appending a suffix after mangling, it just happened earlier. There isn't a good way to "embed" this uniquification into the Itanium mangling scheme, so far as I am aware.

Oh, I was still in favor of adding it after the mangling (the current "mangled.__part.number") but I thought it may be necessary to force mangling on C functions before adding the suffix, if they needed to be demangleable for debug info purposes.

That all said, I don't see any reason to block this decimilization change. It's limited in scope and an obvious improvement.

My concern was that it was layering more workarounds on a patch series that might be going in the wrong direction overall.

Anyway, sounds like, if I'm understanding/tested correctly, that the issue with unmangleability isn't about the symbol name itself but the DW_AT_linkage_name in the DWARF, so it seems like if that isn't present then there's no issue if the real symbol name can't be unmangled/back to the simple name. (but there's some reason the DW_AT_linkage_name, if present, must match the symbol name? (so it can't be the original unsuffixed mangled name))

One reason that DW_AT_linkage_name should be consistent with the real linkage name is to favor AutoFDO. DW_AT_linkage_name is used to generate profiles for inlined functions.

Fri, Jan 8, 1:13 PM · Restricted Project, Restricted Project
dblaikie accepted D93992: [STLExtras] Use return type from operator* of the wrapped iter..

Looks good

Fri, Jan 8, 10:46 AM · Restricted Project

Thu, Jan 7

dblaikie committed rG2ff36e792914: lldb subprogram_ranges.test - remove dependence on temp file name (authored by dblaikie).
lldb subprogram_ranges.test - remove dependence on temp file name
Thu, Jan 7, 8:05 PM
dblaikie committed rG4a3c2ba89046: Fix print-dot-ddg.ll so it doesn't try to write to the source tree (& uses the… (authored by dblaikie).
Fix print-dot-ddg.ll so it doesn't try to write to the source tree (& uses the…
Thu, Jan 7, 7:58 PM
dblaikie committed rG696775d96ecd: Fix subprogram_ranges.test by explicitly using lld (authored by dblaikie).
Fix subprogram_ranges.test by explicitly using lld
Thu, Jan 7, 7:53 PM
dblaikie added a reverting change for rGd2ddc694ff94: Revert "Revert "[analyzer] NFC: Move path diagnostic consumer implementations…: rGb12f26733a42: Revert "Revert "Revert "[analyzer] NFC: Move path diagnostic consumer….
Thu, Jan 7, 6:19 PM
dblaikie committed rGb12f26733a42: Revert "Revert "Revert "[analyzer] NFC: Move path diagnostic consumer… (authored by dblaikie).
Revert "Revert "Revert "[analyzer] NFC: Move path diagnostic consumer…
Thu, Jan 7, 6:19 PM
dblaikie added a comment to D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.

If it's better to write it using C++ source and custom clang flags I can do that instead (it'll be an -mllvm flag - looks like there's one other test that does that: lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py: dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))) - means the test will be a bit more convoluted to tickle the subprogram ranges, but not much - just takes two functions+function-sections.

I certainly wouldn't want to drop the existing test.

Thu, Jan 7, 2:29 PM · Restricted Project
dblaikie committed rG274afac9a17f: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms (authored by dblaikie).
lldb: Add support for DW_AT_ranges on DW_TAG_subprograms
Thu, Jan 7, 2:28 PM
dblaikie closed D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms.
Thu, Jan 7, 2:28 PM · Restricted Project
dblaikie added inline comments to D93433: [IR] Use LLVM_ENABLE_ABI_BREAKING_CHECKS to guard ABI changes..
Thu, Jan 7, 2:19 PM · Restricted Project
dblaikie committed rG3503c856819e: Fixup Asserts+!AbiBreakingChecks fallout from db33f85c7124 (authored by dblaikie).
Fixup Asserts+!AbiBreakingChecks fallout from db33f85c7124
Thu, Jan 7, 2:18 PM