User Details
- User Since
- Oct 8 2012, 9:19 AM (432 w, 1 d)
Today
Haven't reviewed the logic in detail - but generally looks about right to me.
Looks good!
(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)
(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:
A few thoughts:
Yesterday
(Works for me, FWIW - takes my local run down from 42 failures in check-lldb-api to 1 (a timeout))
Sun, Jan 17
Sounds good, thanks!
Sounds good, thanks!
Fri, Jan 15
"But because their source lines are outside the function source range"
Couple of optional pieces.
Thu, Jan 14
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.
Looks worth another go
Thanks for the pointers!
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)
Wed, Jan 13
Sounds good to me - but give it a day or two. @MaskRay or others might want to weigh in.
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.
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
(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)
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)
Looks good, thanks!
Can you commit this yourself (do you have commit access), or do you need someone to commit it for you?
Code change looks good - test might be able to be tweaked a bit before committing.
Sounds good
Tue, Jan 12
I'll leave this one to @aprantl
Sounds good - thanks!
Sounds good
Sounds OK
Seems OK - I don't know if there's a nicer way to handle the latter cases that need the 1 + ~N.
Do you need to run the destructor before placement new in these situations?
Mon, Jan 11
I think this is here for consistency with the other complete* functions - perhaps it could be left as an assertion?
Might still be a useful conversion for possible future callsites? Or are all the call sites pretty clearly not using this?
Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)
Perhaps we could rewrite the code to use ~ or the like instead of unary - if that works?
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?
Looks good - thanks!
Sun, Jan 10
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).
Sure, sounds alright.
Sat, Jan 9
Fri, Jan 8
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.
Looks good - thanks!
Looks good