Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (379 w, 4 d)

Recent Activity

Yesterday

dblaikie committed rG58b10df54ffd: DebugInfo: Move SectionLabel tracking into CU's addRange (authored by dblaikie).
DebugInfo: Move SectionLabel tracking into CU's addRange
Fri, Jan 17, 6:20 PM
dblaikie added inline comments to D72812: [IR] Module's NamedMD table needn't be 'void *'.
Fri, Jan 17, 5:42 PM · Restricted Project
dblaikie committed rG46ed93315fce: [IR] Remove some unnecessary cleanup in Module's dtor, and use a unique_ptr to… (authored by dblaikie).
[IR] Remove some unnecessary cleanup in Module's dtor, and use a unique_ptr to…
Fri, Jan 17, 5:34 PM
dblaikie added a comment to D71932: [DWARF] Better detect errors in Address Range Tables..

Thanks for the review!

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

My main intention is to add support for 64-bit DWARF.

Fri, Jan 17, 5:14 PM · Restricted Project, debug-info
dblaikie accepted D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing.
Fri, Jan 17, 5:14 PM · Restricted Project
dblaikie accepted D72155: [DebugInfo] Make incorrect debug line extended opcode length non-fatal.

Out of curiosity: what's your broader goal with this work? (it'll help understand what's in-scope and out of scope, and better understand the framing when reviewing changes)

There's two parts to this:

  1. Making it easier for consumers to continue and try to do something with slightly bad output. One of the problems with using the unrecoverable errors is that it prevents people even trying to iterate over later tables.
  2. (this one's not specific to this patch, but some other patches in this area I've been doing are related): we have some local code that uses the DebugInfo library to read a line table. In order for this code to be sound, we need to make sure the line table makes sense. The parser in its current state doesn't pick up on a number of bad situations, so we added some local patches to detect these other errors (local because we didn't have time at that point to try to push them into the open source). We'd like to now get these into the opensource library, to avoid merge conflicts.
Fri, Jan 17, 5:05 PM · Restricted Project
dblaikie accepted D71932: [DWARF] Better detect errors in Address Range Tables..

Let's try that again - guess I'd already approved this before.

Fri, Jan 17, 3:20 PM · Restricted Project, debug-info
dblaikie requested changes to D71932: [DWARF] Better detect errors in Address Range Tables..

Sounds good - thanks!

Fri, Jan 17, 3:20 PM · Restricted Project, debug-info
dblaikie accepted D71932: [DWARF] Better detect errors in Address Range Tables..

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

Fri, Jan 17, 3:11 PM · Restricted Project, debug-info
dblaikie accepted D72157: [test][llvm-dwarfdump] Add extra test case for invalid MD5 form.

Looks good - James, I might've asked this on another thread - but could you write up a brief goals/design email of what your overall goal is with these patches? There seem to be enough of them that it'd be helpful to link to in each review (and summarize where the patch fits in the broader scheme of things) & provide framing when reviewing patches to better understand the overall goals.

Fri, Jan 17, 3:02 PM · Restricted Project

Thu, Jan 16

dblaikie accepted D72895: [Transforms][RISCV] Remove a "using namespace llvm" from an include file. Fix a place that became dependent on it..

Ah, bother - good catch!

Thu, Jan 16, 5:56 PM · Restricted Project
dblaikie accepted D72557: Add pretty printers for llvm::PointerIntPair and llvm::PointerUnion..

Looks good - thanks!

Thu, Jan 16, 3:43 PM · Restricted Project
dblaikie added a comment to D72590: NFC: Change PointerLikeTypeTraits::NumLowBitsAvailable from enum to static member. This allows GDB pretty printer to find the value..

I've committed a generalization of this (applying the same logic to all instances of NumLowBitsAvailable) in 65eb74e94b414fcde6bfa810d1c30c7fcb136b77 - so this review can be closed or abandoned.

Thu, Jan 16, 3:43 PM · Restricted Project
dblaikie accepted D71732: Move the sysroot attribute from DIModule to DICompileUnit.

Looks good - thanks!

Thu, Jan 16, 3:43 PM · Restricted Project, Restricted Project, Restricted Project, debug-info
dblaikie committed rG65eb74e94b41: PointerLikeTypeTraits: Standardize NumLowBitsAvailable on static constexpr… (authored by dblaikie).
PointerLikeTypeTraits: Standardize NumLowBitsAvailable on static constexpr…
Thu, Jan 16, 3:34 PM
dblaikie added a comment to D72590: NFC: Change PointerLikeTypeTraits::NumLowBitsAvailable from enum to static member. This allows GDB pretty printer to find the value..

I've committed a generalization of this (applying the same logic to all instances of NumLowBitsAvailable) in 65eb74e94b414fcde6bfa810d1c30c7fcb136b77 - so this review can be closed or abandoned.

Thu, Jan 16, 3:34 PM · Restricted Project
dblaikie added a comment to D72828: [DWARF5] Added support for emission of debug_macro section..

Please implement the dwarfdump support first (this patch can be reviewed during that time - but shouldn't be committed without tests & ideally those tests should be written using dwarfdump (sometimes there are exceptions for especially complicated dumping situations, where raw assembly testing is used instead, but I don't think it's necessary to make that tradeoff here))

Thu, Jan 16, 10:13 AM · Restricted Project, debug-info

Wed, Jan 15

dblaikie accepted D72812: [IR] Module's NamedMD table needn't be 'void *'.

Sounds good -t hanks for all the context. Previously this might've been an effort to keep the dependencies of a core header like Module.h small, but since it now has the dependencies this relies on - this change isn't making the situation worse in any way I can think of.

Wed, Jan 15, 3:24 PM · Restricted Project
dblaikie accepted D72804: [xray] add --symbolize-mangled cli opt for llvm-xray extract to output mangled names.

Looks good -t hanks!

Wed, Jan 15, 1:59 PM · Restricted Project
dblaikie added a comment to D72804: [xray] add --symbolize-mangled cli opt for llvm-xray extract to output mangled names.

The usual "naming of things" - are there flags with similar functionality in other tools (like llvm-symbolizer) that might provide inspiration for consistent naming here (perhaps the name you've chosen already was inspired by that)?

Wed, Jan 15, 1:19 PM · Restricted Project
dblaikie added a comment to D71451: Support to emit extern variables debuginfo with "-fstandalone-debug".

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

For above case clang size is increase because ,it is difference between clang build without "-fstandalone-debug" option and clang build with "-fstandalone-debug" option and both build contain change D71451 and D71599 . So for clang build with "-fstandalone-debug" option size will be more because it will add debuginfo.

And to check impact of my change on clang i have build clang with and without D71451 and D71599 change(testcases are not included).

Size of clang without D71451 and D71599 change and with option "-fstandalone-debug":-
=======================================================================

.comment 159 0
.debug_str 3994952 0
.debug_loc 941 0
.debug_abbrev 12754 0
.debug_info 2223641 0
.debug_ranges 46592 0
.debug_line 153901 0
.note.gnu.gold-version 28 0
Total 6827932

Size of clang with D71451 and D71599 change and with option "-fstandalone-debug":-
======================================================================

.comment 159 0
.debug_str 3994894 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2223617 0
.debug_ranges 46592 0
.debug_line 153865 0
.note.gnu.gold-version 28 0
Total 6827806

Size of clang with D71451 and D71599 is reduced.

Wed, Jan 15, 12:05 PM · Restricted Project, Restricted Project, debug-info

Tue, Jan 14

dblaikie added inline comments to D71732: Move the sysroot attribute from DIModule to DICompileUnit.
Tue, Jan 14, 2:17 PM · Restricted Project, Restricted Project, Restricted Project, debug-info
dblaikie accepted D72489: [DWARF] Emit DW_AT_call_return_pc as an address.

While the related design discussions continue - the patch itself is good/correct & there's nothing much to be done about the address pool size/relocations increase for now, for GDB at least, which is what I care about.

Tue, Jan 14, 1:56 PM · Restricted Project, Restricted Project
dblaikie added a comment to D72624: [WIP] TargetMachine Hook for Module Metadata.

(just a general comment that this code review should be only in service of the design discussion happening on llvm-dev - please don't approve/commit this without closing out the design discussion there if there are actionable conclusions from this review)

Tue, Jan 14, 1:47 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added inline comments to D72557: Add pretty printers for llvm::PointerIntPair and llvm::PointerUnion..
Tue, Jan 14, 1:41 PM · Restricted Project
dblaikie accepted D69779: -fmodules-codegen should not emit extern templates.

Looks good - thanks for your patience!

Tue, Jan 14, 1:38 PM · Restricted Project
dblaikie added a comment to D71732: Move the sysroot attribute from DIModule to DICompileUnit.

Some mysterious CFI failures I haven't looked into yet, but were pretty clearly blamed on this change & went away with a revert.

Because the flag was previously not (de)serialized and now shows up in LTO builds where it was dropped before?

Tue, Jan 14, 12:21 PM · Restricted Project, Restricted Project, Restricted Project, debug-info
dblaikie added inline comments to D72589: Add GDB pretty printers for llvm::ilist, llvm::simple_ilist, and llvm::ilist_node..
Tue, Jan 14, 8:59 AM · Restricted Project
dblaikie accepted D72695: [MachO] Add a test for detecting reserved unit length..
Tue, Jan 14, 8:22 AM · Restricted Project, lld
dblaikie added inline comments to D72557: Add pretty printers for llvm::PointerIntPair and llvm::PointerUnion..
Tue, Jan 14, 8:12 AM · Restricted Project
dblaikie added inline comments to D72557: Add pretty printers for llvm::PointerIntPair and llvm::PointerUnion..
Tue, Jan 14, 8:12 AM · Restricted Project
dblaikie added a comment to D72590: NFC: Change PointerLikeTypeTraits::NumLowBitsAvailable from enum to static member. This allows GDB pretty printer to find the value..

Could you provide a small example of the problem here? LLVM does emit debug info for used enums (granted, if it's used as a template parameter it looks like LLVM does not emit them - but if it's used in a runtime evaluated expression it does/should & it looks like NumLowBitsAvailable is used in that way pretty regularly)

Tue, Jan 14, 7:52 AM · Restricted Project

Mon, Jan 13

dblaikie accepted D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes..

Looks good - thanks!

Mon, Jan 13, 4:23 PM · debug-info, Restricted Project
dblaikie added a comment to D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes..
In D72427#1818322, @rnk wrote:

Total object file size on Windows, compiling with RelWithDebInfo:

before: 4,257,448 kb
after: 2,104,963 kb

And on Linux

before: 9,225,140 kb
after: 4,387,464 kb

These numbers are amazing!

I made a summary of Amy's list of types that become incomplete here: https://reviews.llvm.org/P8184
It collapses template parameters and counts up instantiations.

Mon, Jan 13, 4:22 PM · debug-info, Restricted Project
dblaikie added a comment to D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes..

Looks pretty good to me - if you could commit the debug info level refactoring separately/up-front, and maybe the test case could be simplified a bit. Looking forward to seeing what comes of this option, analysis of missing types, etc.

Mon, Jan 13, 3:44 PM · debug-info, Restricted Project
dblaikie added inline comments to D69779: -fmodules-codegen should not emit extern templates.
Mon, Jan 13, 2:58 PM · Restricted Project

Sun, Jan 12

dblaikie added a comment to D72565: adding __has_feature support for left c++ type_traits.

Probably needs test coverage (perhaps check the commits that added other feature tests - there's probably somewhere you can plugin tests in a similar way to plugging in the feature tests themselves)

Sun, Jan 12, 9:26 AM · Restricted Project

Fri, Jan 10

dblaikie added a comment to D72489: [DWARF] Emit DW_AT_call_return_pc as an address.

I ran some size analysis (& hadn't realized that this special case was only done in the DWARFv5 codepath... that leads me to wonder about size impact in DWARFv4, so maybe I'll need to go back and run some general size analysis cost of these call sites).

Fri, Jan 10, 11:55 AM · Restricted Project, Restricted Project
dblaikie added inline comments to D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes..
Fri, Jan 10, 10:31 AM · debug-info, Restricted Project
dblaikie accepted D70524: Support DebugInfo generation for auto return type for C++ functions..

Looks good - thanks!

Fri, Jan 10, 9:27 AM · debug-info, Restricted Project, Restricted Project
dblaikie accepted D72337: [DebugInfo][Support] Replace DWARFDataExtractor size function.

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Ah, I was more expecting an end-to-end test (with llvm-dwarfdump or similar) when you said in the patch description "The old behaviour could cause an assertion in the debug line parser"

What were the circumstances where this assertion would fail? Was the failure reachable with llvm-dwarfdump, or only via a unit test/non-production API usage?

Oh, sorry for misunderstanding. We have a local test for a local patch in LLD that makes some use of the debug information, but this patch doesn't create the data extractor with a section, which meant that it ran into this assertion. I checked, and I can't find any other usage of this function with a data extractor constructed without a section.

Fri, Jan 10, 7:36 AM · Restricted Project

Thu, Jan 9

dblaikie added a comment to D72489: [DWARF] Emit DW_AT_call_return_pc as an address.

Thanks for looking into this!

Thu, Jan 9, 7:21 PM · Restricted Project, Restricted Project
dblaikie accepted D72443: [DebugInfo] Improve error message text.

Seems good

Thu, Jan 9, 4:55 PM · Restricted Project
dblaikie added a comment to D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing.

As mentioned in another review - I'm not sure "assume bigger is correct" is a great strategy & not sure there's a lot of value in continuing in the face of a conflict like that. What sort of situations really benefit from such parsing optimism?

Thu, Jan 9, 4:47 PM · Restricted Project
dblaikie added a comment to D72157: [test][llvm-dwarfdump] Add extra test case for invalid MD5 form.

I guess these error messages could be more precise about what's invalid & why it's invalid. In this case I guess it's invalid because the DW_LNCT_MD5 has a form that contains only one byte, not large enough to encode a whole MD5 sum?

Thu, Jan 9, 4:44 PM · Restricted Project
dblaikie added a comment to D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes..

What's the plan for this? Is it still in an experimental stage, with the intent to investigate the types that are no longer emitted unedr the flag & explain why they're missing (& either have a justification for why that's acceptable, or work on additional heuristics to address the gaps?)?

If so, I'd probably rather this not be a full driver flag - if it's a reliable way to reduce debug info size (like the existing heuristics under -fstandalone-debug*) it should be rolled into -fno-standalone-debug behavior, and if it's not fully fleshed out yet, I think an -Xclang flag would be more suitable for the experimental phase.

Pretty much, I think the plan is to investigate further and maybe have more people try it. The -Xclang flag seems reasonable. Do you have thoughts on whether the added DebugInfoKind level makes sense?

Thu, Jan 9, 4:31 PM · debug-info, Restricted Project
dblaikie added a comment to D72155: [DebugInfo] Make incorrect debug line extended opcode length non-fatal.

Out of curiosity: what's your broader goal with this work? (it'll help understand what's in-scope and out of scope, and better understand the framing when reviewing changes)

Thu, Jan 9, 4:29 PM · Restricted Project
dblaikie added a comment to D72337: [DebugInfo][Support] Replace DWARFDataExtractor size function.

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Thu, Jan 9, 2:47 PM · Restricted Project
dblaikie accepted D71931: [DWARF] Allow empty address range tables..

Padding stuff seems like overkill to me, hardcoding the actual value seems like it'd probably be fine, but I don't mind so much.

Thu, Jan 9, 2:28 PM · Restricted Project, debug-info
dblaikie added a comment to D71732: Move the sysroot attribute from DIModule to DICompileUnit.
Thu, Jan 9, 2:19 PM · Restricted Project, Restricted Project, Restricted Project, debug-info
dblaikie added a comment to D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes..

What's the plan for this? Is it still in an experimental stage, with the intent to investigate the types that are no longer emitted unedr the flag & explain why they're missing (& either have a justification for why that's acceptable, or work on additional heuristics to address the gaps?)?

Thu, Jan 9, 10:14 AM · debug-info, Restricted Project

Wed, Jan 8

dblaikie added inline comments to D70524: Support DebugInfo generation for auto return type for C++ functions..
Wed, Jan 8, 2:33 PM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D72337: [DebugInfo][Support] Replace DWARFDataExtractor size function.

The old behaviour could cause an assertion in the debug line parser

Wed, Jan 8, 10:21 AM · Restricted Project
dblaikie added reviewers for D71593: [DebugInfo] Call site entries cannot be generated for FrameSetup calls: aprantl, JDevlieghere.
Wed, Jan 8, 10:11 AM · debug-info, Restricted Project
dblaikie added a comment to D71593: [DebugInfo] Call site entries cannot be generated for FrameSetup calls.

What about going the other way, and making sure instructions in frame setup that need labels get them?

Wed, Jan 8, 10:11 AM · debug-info, Restricted Project
dblaikie edited reviewers for D72402: [build][NFC] limit parallel link to reduce memory usage, added: rnk; removed: chandlerc.
Wed, Jan 8, 9:05 AM · Restricted Project
dblaikie added a comment to D72402: [build][NFC] limit parallel link to reduce memory usage.

Could we do this the other way around - and "AND LINKER_IS_GNULD"? Because so far as I know gold has reasonable memory usage too (& LD64), etc. I /think/ it's just GNULD that's the outlier here. (I can't speak for SOLARISLD, admittedly)

Wed, Jan 8, 9:05 AM · Restricted Project
dblaikie accepted D72399: [Support] Replace Windows __declspec(thread) with thread_local in LLVM_THREAD_LOCAL.

Sounds good to me - but probably good to get someone who works on the MSVC platform (like @rnk ) to confirm.

Wed, Jan 8, 8:09 AM · Restricted Project

Tue, Jan 7

dblaikie added a reviewer for D72321: Add test for GDB pretty printers.: aprantl.
Tue, Jan 7, 2:35 PM · Restricted Project
dblaikie updated subscribers of D72321: Add test for GDB pretty printers..

Seems OK to me, but @aprantl - could you take a look too since I don't often use/interact with the debuginfo-tests. Do you think this is something worth having/appropriate to have there?

Tue, Jan 7, 2:35 PM · Restricted Project
dblaikie accepted D72331: OpaquePtr: add type to inalloca attribute..

Looks pretty good to me - couple of minor things. If you want to check with @rnk (best point of contact on the inalloca feature in general), that'd probably be good.

Tue, Jan 7, 1:46 PM · Restricted Project, Restricted Project

Mon, Jan 6

dblaikie added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I think the question I have: What is the value (I don't mean to assume there is no value, but I want to understand the specific use case) of supporting this if the full generality of symbol interposition is not supported? (is there a well defined use case that is usable with this patch and doesn't need the full generality? Or is the use case going to be subtly broken if it changes even slighlty and happens to trip over another part of LLVM that doesn't support symbol interposition? (in which case this is a great patch, but only as part of an effort to fully support the general case))

@dblaikie I came from a ppc64 use case https://reviews.llvm.org/D71639#1803561 where I could not construct a good test due to the MC issue this patch intends to fix. The advantages include at least the following:

  • Improve compatibility with GNU as. This improves portability of hand-written assembly.
Mon, Jan 6, 2:25 PM · Restricted Project
dblaikie added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

I had a talk about this once I believe with @hfinkel (or maybe @dblaikie), and this behavior was intentional. Essentially LLVM may perform IPO between the 2 functions, in which case its no longer appropriate to allow the callee to be preempted at runtime. The back-ends aren't consistent on this behaviour either. For example:

__attribute__((noinline))
int bar(int i) {
    return i;
}


int caller(int i) {
    if (bar(i) == i)
      return 0;

    return 1;
}

Compiling this for PowerPC and AArch64 Linux and -fPIC I get no relocation for the call. Compiling for X86_64 Linux and -fPIC I do get a relocation for the call. So it seems to be pick your poison: Emit a relocation and expose a potential runtime problem, or don't emit a relocation and break ELF interposition semantics. I'm also not sure if the optimizer actually skips all IPO when the functions are in different sections, so we might end up with both broken shared object interposition and potentially unsafe codegen 😦

Behavior without this patch.

  • clang -target {aarch64,ppc64le} -fPIC -c => no relocation referencing bar
  • clang -target {aarch64,ppc64le} -fPIC -c -ffunction-sections => a branch relocation referencing bar

    Look, we are already inconsistent. With this patch, we will consistently emit a relocation. I thinks this patch makes the situation better.
Mon, Jan 6, 11:22 AM · Restricted Project

Sat, Jan 4

dblaikie accepted D72156: [test][DebugInfo][NFC] Rename method for clarity.

Bit verbose, but sounds alright.

Sat, Jan 4, 2:05 PM · Restricted Project

Fri, Jan 3

dblaikie accepted D72136: Add gdb pretty printer for MutableArrayRef, remove ConstArrayRef..

Ah, thanks for that - seems I wrote this as I had /wanted/ ArrayRef to be implemented, rather than how it ended up being implemented. (when MutableArrayRef was proposed, I had proposed changing ArrayRef to be mutable and introducing ConstArrayRef but folks didn't want the churn - guess that stuck in my head a bit).

Fri, Jan 3, 9:44 PM · Restricted Project
dblaikie added a comment to D70600: [Error] Add stack traces for llvm::Error invariant violations..

FWIW working on a codebase that heavily uses both Error and threads, the side-table seems fairly a bit terrifying. Even if the thread safety issues are fixed, do we really want to pay for mutex lock/unlock to create errors, to save a pointer?

thread_local might be an option, though as I understand llvm/Support may have to be portable to environments where that doesn't work well.

An atomic pointer would work too. This cost is only paid on failure values and these are already heap allocated, so I'm I wouldn't expect the overhead to be prohibitive either way.

I don't follow this - are you saying if heap allocation is OK, locking a mutex must be too, as it's cheaper?
Locking/unlocking a shared global mutex can certainly be more expensive than malloc if there's a lot of contention. malloc implementations typically go to some lengths to avoid a global lock for this reason.

Example workload: clangd typically runs ~30 threads for background indexing, each thread frequently calls functions returning Expected<T> and recovers from errors (Error is documented as being for recoverable errors). It'd be a big surprise if this sort of use of Error led to contention/serialization on these worker threads.

Note this feature's only intended to be opt-in for debugging purposes, so for production use cases it shouldn't be a huge deal (though could be cheaper/free if it was a build-time setting rather than something you could opt into/out of at runtime - perhaps that's the point you're making? Though we do check various globals (cl::opts, etc) in a fair number of codepaths, I think, such that adding one doesn't seem likely to be super problematic?). (as much as I agree with you that I'd rather not have a side table)

Part of the reason for this patch was a need to provide file/line number for errors that lead to aborts via llvm::cantFail. This can be very important when debugging failing bots for which you have not access and are unable to reproduce. Since this solution is too expensive to turn on all the time, it's doubtful it'll be enabled on any bots, which makes it useless for my primary use case.

That doesn't mean it wouldn't be useful in cases where you can reproduce locally, but that wasn't my initial motivation. I'm also wary of the side-table.

Fri, Jan 3, 7:45 PM · Restricted Project
dblaikie added a comment to D70600: [Error] Add stack traces for llvm::Error invariant violations..

FWIW working on a codebase that heavily uses both Error and threads, the side-table seems fairly a bit terrifying. Even if the thread safety issues are fixed, do we really want to pay for mutex lock/unlock to create errors, to save a pointer?

thread_local might be an option, though as I understand llvm/Support may have to be portable to environments where that doesn't work well.

An atomic pointer would work too. This cost is only paid on failure values and these are already heap allocated, so I'm I wouldn't expect the overhead to be prohibitive either way.

I don't follow this - are you saying if heap allocation is OK, locking a mutex must be too, as it's cheaper?
Locking/unlocking a shared global mutex can certainly be more expensive than malloc if there's a lot of contention. malloc implementations typically go to some lengths to avoid a global lock for this reason.

Example workload: clangd typically runs ~30 threads for background indexing, each thread frequently calls functions returning Expected<T> and recovers from errors (Error is documented as being for recoverable errors). It'd be a big surprise if this sort of use of Error led to contention/serialization on these worker threads.

Fri, Jan 3, 3:09 PM · Restricted Project
dblaikie added a comment to D70628: [Support] Enable file + line info in LLVM stack traces on Darwin..

...
I think the best long term solution to this problem is to teach llvm-symbolize how to cope with debug info in objects, rather than requiring a dSYM. An intermediate step (possibly generically useful) would be to add an option to llvm-symbolize to run llvm-dsymutil if there's no dSYM available.

I suppose that might be useful for individuals, but I don't think it would be a good idea for the bots, which is my primary target.

My last comment concerned the intermediate step. I think your long-term solution would be great.

If the option is added (and LLVM's stack symbolication call-out knows to use it) then I think it should work on the bots too, right? The only issue would be that crashing test cases would be slightly slower (since we'd have to produce the dSYMs).

Fri, Jan 3, 2:50 PM · Restricted Project

Thu, Jan 2

dblaikie added a comment to D70600: [Error] Add stack traces for llvm::Error invariant violations..

Oh, I remember part of my motivation for designing it this way: While failure values are expected to be expensive, we don't want to make them unnecessarily expensive, which is why the error-geneation-point trace is under a flag: You don't want to stack-trace every error that is ever generated. However, making this conditional means we need to check a global flag to decide whether we *should* do the stack trace. Once you're doing that, you may as well make your "flag" a pointer to the side table.

Thu, Jan 2, 5:19 PM · Restricted Project
dblaikie accepted D72024: Add C source to two debug info tests..
Thu, Jan 2, 2:13 PM · Restricted Project
dblaikie accepted D72023: Change dbg-*-tag-offset tests to use llvm-dwarfdump..
Thu, Jan 2, 2:09 PM · Restricted Project
dblaikie added a comment to D72023: Change dbg-*-tag-offset tests to use llvm-dwarfdump..

Might want to use CHECK-NEXT as you were before and/or maybe some CHECK-NOT: DW_TAG between the TAG/AT/AT triples, to make sure you aren't checking attributes in some unintended TAG, etc.

Thu, Jan 2, 1:09 PM · Restricted Project
dblaikie added inline comments to D72024: Add C source to two debug info tests..
Thu, Jan 2, 1:08 PM · Restricted Project
dblaikie committed rGd45b394b3e67: Polly: Fix a tag type mismatch (struct/class) (authored by dblaikie).
Polly: Fix a tag type mismatch (struct/class)
Thu, Jan 2, 12:21 PM
dblaikie added a comment to D69779: -fmodules-codegen should not emit extern templates.

Hey - thanks for this! Does look like it reproduces in modules:

Thu, Jan 2, 12:12 PM · Restricted Project
dblaikie added a comment to D71839: [Dsymutil][Debuginfo][NFC] Reland: Refactor dsymutil to separate DWARF optimizing part. #2..
In D71839#1795483, @avl wrote:

Not sure if this is better/worse off as a subdirectory of lib/CodeGen, rather than as a top level library on its own (lib/DWARFOptimizer). Figured I'd mention it, but mostly leave it up to the other dsymutil folks to make the choices here.

My idea was that DWARFOptimizer does generation task. F.e. AsmPrinter - generates asm and it is under CodeGen. DWARFOptimizer generates optimized DWARF. That is why I put it under CodeGen.
Though if it is better to put it on top level - lib/DWARFOptimizer - I am OK to do this.

@aprantl @JDevlieghere @probinson any preferences here? If I've got to make the call I'll probably pick lib/DWARFOptimizer or lib/DWARFLinker (probably the latter)

My personal preference goes to lib/DWARFLinker.

Thu, Jan 2, 11:24 AM · debug-info, Restricted Project

Tue, Dec 31

dblaikie added a comment to D72032: [llvm-exegesis] Add pfm counters for Zen2 (znver2)..

I take it there's no way to write a portable test for this patch? (one that doesn't have to run on the specific hardware, but still demonstrates the presence of this newly added support?)

Tue, Dec 31, 8:44 AM · Restricted Project

Mon, Dec 30

dblaikie committed rGb350c666ab65: Revert "DebugInfo: Fix rangesBaseAddress DICompileUnit bitcode… (authored by dblaikie).
Revert "DebugInfo: Fix rangesBaseAddress DICompileUnit bitcode…
Mon, Dec 30, 11:02 PM
dblaikie added a reverting change for rGc51b45e32ef7: DebugInfo: Fix rangesBaseAddress DICompileUnit bitcode…: rGb350c666ab65: Revert "DebugInfo: Fix rangesBaseAddress DICompileUnit bitcode….
Mon, Dec 30, 11:02 PM
dblaikie updated subscribers of D71839: [Dsymutil][Debuginfo][NFC] Reland: Refactor dsymutil to separate DWARF optimizing part. #2..
In D71839#1795483, @avl wrote:

Not sure if this is better/worse off as a subdirectory of lib/CodeGen, rather than as a top level library on its own (lib/DWARFOptimizer). Figured I'd mention it, but mostly leave it up to the other dsymutil folks to make the choices here.

My idea was that DWARFOptimizer does generation task. F.e. AsmPrinter - generates asm and it is under CodeGen. DWARFOptimizer generates optimized DWARF. That is why I put it under CodeGen.
Though if it is better to put it on top level - lib/DWARFOptimizer - I am OK to do this.

Mon, Dec 30, 10:23 AM · debug-info, Restricted Project

Fri, Dec 27

dblaikie committed rG22f34c7f34ad: lld: Remove explicit copy ops from AssociatedIterator, relying on implicit… (authored by dblaikie).
lld: Remove explicit copy ops from AssociatedIterator, relying on implicit…
Fri, Dec 27, 5:30 PM
dblaikie committed rGc51b45e32ef7: DebugInfo: Fix rangesBaseAddress DICompileUnit bitcode… (authored by dblaikie).
DebugInfo: Fix rangesBaseAddress DICompileUnit bitcode…
Fri, Dec 27, 5:30 PM
dblaikie added inline comments to D54242: DebugInfo: Add a CU metadata attribute for use of DWARF ranges base address specifiers.
Fri, Dec 27, 5:30 PM · Restricted Project
dblaikie added a comment to D71546: [MachO] Fix detecting malformed DWARF..

Was this fixing an existing buildbot/test failure? (if not, it should probably have a test? If it did fix an existing failure - could you mention what that failure was?)

Fri, Dec 27, 2:37 PM · Restricted Project, lld
dblaikie added inline comments to D70753: hwasan: add tag_offset DWARF attribute to optimized debug info.
Fri, Dec 27, 2:28 PM · Restricted Project
Herald added a reviewer for D70629: llvm-symbolizer: Support loclist in FRAME.: jhenderson.
Fri, Dec 27, 1:33 PM · Restricted Project
Herald added a reviewer for D70630: llvm-symbolizer: fix handling of DW_AT_specification in FRAME.: jhenderson.
Fri, Dec 27, 1:33 PM · Restricted Project
dblaikie added a comment to D71026: Fix LLVM_ENABLE_MODULES=ON + BUILD_SHARED_LIBS=ON build.

If I build on macOS with cmake -GNinja -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_PROJECTS=llvm;clang;lld;compiler-rt;libcxx;libcxxabi -DBUILD_SHARED_LIBS=ON -DLLVM_ENABLE_MODULES=ON, I get lots of linker errors building e.g. libclangASTDiff.dylib due to using the internal ASTMatchers header inside the Tooling/Transformer headers.

This is probably due to https://bugs.llvm.org/show_bug.cgi?id=23521 mentioned in the previous commits that added the exclude directives.

Fri, Dec 27, 12:56 PM · Restricted Project
dblaikie committed rGd8018233d1ea: Revert "CWG2352: Allow qualification conversions during reference binding." (authored by dblaikie).
Revert "CWG2352: Allow qualification conversions during reference binding."
Fri, Dec 27, 12:28 PM
dblaikie added a reverting change for rGde21704ba96f: CWG2352: Allow qualification conversions during reference binding.: rGd8018233d1ea: Revert "CWG2352: Allow qualification conversions during reference binding.".
Fri, Dec 27, 12:28 PM
dblaikie added inline comments to D71876: [DWARF] Support DWARF64 in DWARFDebugArangeSet..
Fri, Dec 27, 11:30 AM · Restricted Project, debug-info
dblaikie added inline comments to D71875: [DWARF] Return Error from DWARFDebugArangeSet::extract()..
Fri, Dec 27, 11:21 AM · Restricted Project, debug-info
dblaikie added a comment to D71932: [DWARF] Better detect errors in Address Range Tables..

Could these tests all be in one run? (ie: even though the length is "invalid" in terms of parsing the contents of the section - but valid enough to jump to the next contribution and parse that - except, I guess, at an invalid 64 bit length perhaps, because that might be too long to be bothered testing - so perhaps that could be the last test case in the file)

Fri, Dec 27, 11:11 AM · Restricted Project, debug-info
dblaikie added inline comments to D71931: [DWARF] Allow empty address range tables..
Fri, Dec 27, 11:02 AM · Restricted Project, debug-info

Thu, Dec 26

dblaikie accepted D67097: [DWARF] Check for format mismatch between CU and Range List Table..

yeah, as @probinson said, at some point someone could add a callback error filter here to decide what's a fatal error and what isn't - but this seems reasonable for now

Thu, Dec 26, 8:41 AM · Restricted Project, debug-info

Wed, Dec 25

dblaikie accepted D71876: [DWARF] Support DWARF64 in DWARFDebugArangeSet..

Looks good - thanks!

Wed, Dec 25, 10:29 AM · Restricted Project, debug-info
dblaikie accepted D71875: [DWARF] Return Error from DWARFDebugArangeSet::extract()..
Wed, Dec 25, 10:29 AM · Restricted Project, debug-info

Tue, Dec 24

dblaikie committed rGfccac1ec1695: DebugInfo: Correct the form of DW_AT_macro_info in .dwo files (sec_offset… (authored by dblaikie).
DebugInfo: Correct the form of DW_AT_macro_info in .dwo files (sec_offset…
Tue, Dec 24, 1:30 AM
dblaikie committed rG83c7a424d968: DebugInfo: Add {} to address -Wdangling-else warning. (authored by dblaikie).
DebugInfo: Add {} to address -Wdangling-else warning.
Tue, Dec 24, 1:21 AM

Mon, Dec 23

dblaikie added a comment to D70696: [DebugInfo] Support to emit debugInfo for extern variables.

@dblaikie The root cause has been identified by @rnk . A clang plugin is used which requires the following patch:

https://github.com/llvm/llvm-project/commit/5128026467cbc17bfc796d94bc8e40e52a9b0752

which has been merged. The original extern variable debug info type has been relanded. So we are good. Thanks!

Mon, Dec 23, 11:03 PM · Restricted Project, Restricted Project, debug-info