In D59008#1442256, @t-tye wrote:

In D59008#1442014, @dblaikie wrote:

In D59008#1441903, @t-tye wrote:

LGTM

Do we know the state of split DWARF and DWARF compression for DWARF 5 (compared to DWARF 2)?

State of them in what sense? Compression is pretty orthogonal to any DWARF version - it's more about the container (ELF, etc) you use. Split DWARF is non-standardly supported in pre-v5, and I think it's functioning in the standards conformant v5 mode too.

I had heard mention that at least split DWARF may not be fully supported yet for DWARF 5 in LLVM. But maybe that is old news:-)

In D59008#1441903, @t-tye wrote:

LGTM

Do we know the state of split DWARF and DWARF compression for DWARF 5 (compared to DWARF 2)?

Assuming you've checked this is unused in other projects like lldb, lld, etc, sounds good to me - thanks!

In D58848#1440161, @avl wrote:

Please move this down at least far enough that it doesn't involve separately
computing the file name and opening the file. Where it's a "well, if you didn't
specify a section index, we'll use the first section that covers this address".

It looks like this version of patch does exactly this.
It does not open file separately and does not parse file name.
The only thing which is additionally done is for already opened file :
"well, if you didn't specify a section index, we'll use the first section that covers this address" :

uint64_t SymbolizableObjectFile::getModuleSectionIndexForAddress(

  uint64_t Address) const {
 
for (SectionRef Sec : Module->sections()) {
  if (!Sec.isText() || Sec.isVirtual())
    continue;
 
  if (Address >= Sec.getAddress() &&
      Address <= Sec.getAddress() + Sec.getSize()) {
    return Sec.getIndex();
  }
}
 
return object::SectionedAddress::UndefSection;

}

I think it looks very close to above description.

In D58848#1430203, @avl wrote:

In case Undef specified then there could be several sections with matching address ranges.
It seems to me that DWARFDebugLineTable improper place to make such a decision - which of them should be taken as a result.
This in fact high level decision, not low level.

Pleasue include mention of the bug (PR40276) in the commit message & clarify that while this is useful for some remote compilation models, it's not strictly necessary/the only way to do it (a remote compilation model that keeps relative paths and uses compilation-dir instead can work without this attribute)

In D59553#1436890, @probinson wrote:

Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.

Re. the underlying DWARF issues:
I can't remember whether the DWARF committee has ever considered specifying a special value for "address does not exist" but I doubt it, because the DWARF spec states "If an entity has no associated machine code, none of these [address-related] attributes are specified." (DWARF v5 section 2.17, p.51.) So, the current position of the DWARF spec is that the linker is supposed to rewrite the DWARF, I guess.

This seems like it might be worth a broader discussion about these sort of cases - probably llvm-dev, maybe with some DWARF folks (though the usual LLVM debug info cabal (myself, @aprantl, @probinson, @JDevlieghere, and @echristo) is probably sufficient to get a rough idea of what might be a good general approach).

(also the title of this patch makes it sound like this is a patch to llvm-symbolizer (which I Suspect it should be - if gold/binutils ld produce similar output to lld's current (pre-patch) output, then llvm-symbolizer should be adjusted to cope with that, even if lld is changed))

What do other linkers (gold and gnu binutils, for instance) do in this case?

In D59417#1431153, @vsk wrote:

In D59417#1431111, @dblaikie wrote:

In D59417#1431042, @probinson wrote:

I believe this is a patch to fix PR37964?

Hrm - if debugify diagnoses all instances of instructions without locations as buggy, that sounds very noisy to me. How does it handle all the other cases of merged locations that end up with no location? (if my recollection is accurate and the code is still doing this - merged locations that aren't the same and that aren't calls get no location, not a zero location)

It looks like DILocation::getMergedLocation() always returns a zero location, and that Instruction::applyMergedLocation() no longer restricts itself to calls. This changed with D45396+D45397: we wanted mem2reg to set merged locations on phis to help users narrow down the scope of a crashing instruction.

In D59417#1431042, @probinson wrote:

I believe this is a patch to fix PR37964?

Does this need to be line zero, or could it be no location (the absence of a dbgloc)?

Thanks a bunch - looks good to me.

In D58848#1430071, @avl wrote:

I moved handling Undef case from Symbolizer into SymbolizableObjectFile. Please check whether it is proper place.

Hmm, trying to stare at this function it's confusing me a fair bit. I'd expect this to be more obvious/legible than it is, but it's possible I'm just misunderstanding.

In D58712#1428691, @sarveshtamba wrote:

@dblaikie, anyone you know who can help here?

Would it be simpler/better to revert all the FlagTrivial work? & use the FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been in-tree longer)

In D59272#1426970, @aprantl wrote:

In D59272#1426904, @rnk wrote:

This makes me nervous. The pair of the DIVariable and DILocation is used to uniquely identify the variable that a dbg.value applies to. In practice, inlining is usually what creates multiple instances of the same variable that get updated by dbg.values. You can see how the location is used in DwarfDebug::collectVariableInfoFromMFTable.

Last time I checked, the file, line number and column of a dbg.value was completely unused. To uniquely identify and inlined instance of a variable you need to the combination of inlinedAt field of the dbg.value's DILocation and the dbg.value's DILocalVariable.

I see other places in the code that seem to assume that the scope of the location of a dbg.value will get the scope in which the variable is declared. Your change breaks that invariant. I think we could update such code to get the scope from the DIVariable, but maybe that would be wrong since it would lack inlining information.

Isn't the scope coming from the DILocalVariable though?

In D58712#1427312, @sarveshtamba wrote:

Any thoughts/inputs on the possible causes?

Great - thanks for your patience!

Looks good - thanks!

Looks good to me, thanks

In D58998#1422074, @tkoeppe wrote:

In D58998#1421799, @dblaikie wrote:

In D58998#1419205, @jyknight wrote:

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

The only reason I can think of is that we wanted to keep the overload set small (or trivial), but I don't know if that's significant. (Richard said he doesn't care either way.)

What shall we do now? Would you like to take this change and then refactor, or immediately change this to overloads?

Seems reasonable to me (but feel free to wait for someone with more context/domain awareness)

In D58952#1421508, @probinson wrote:

In D58952#1420777, @dblaikie wrote:

In D58952#1420776, @probinson wrote:

I'm not sure I follow this - the line table maps instructions to source locations. If there are no instructions there's nothing to map

That's the intent. However, in practice it maps instruction *addresses* to source locations.

Well, in practice it's unspecified what it means - so we can look at this and come up with different interpretations. Mine is to interpret these as half open intervals - in which case [100, 100) is empty and you keep searching, then you find [100, 104) and that contains the address you're looking for.

Understood, but if you're walking through the line-number program looking for address 100, and you find it as an exact match, on what grounds do you require the consumer to continue looking?

In D58998#1419205, @jyknight wrote:

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

In D58952#1420776, @probinson wrote:

I'm not sure I follow this - the line table maps instructions to source locations. If there are no instructions there's nothing to map

That's the intent. However, in practice it maps instruction *addresses* to source locations.

Might be worth a separate/targeted test with some of the interesting edge cases? (like empty range at the end of the sequence, etc)

Mind adding at least one example with neither triviality flag? (I guess a type that's not a struct or class? A union maybe?)

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

In D58712#1417984, @sarveshtamba wrote:

Hi @dblaikie,

While building Apple Swift 5.0 toolchain on PowerPC64LE, we hit up on the following error(details in first comment) while going through the step "--- Installing swiftpm ---" i.e. installation of the swift package manager:-

		swift: /home/sar/swift-source/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:12111: llvm::SDValue combineBVOfConsecutiveLoads(llvm::SDNode *, llvm::SelectionDAG &): Assertion `!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) && "The loads cannot be both consecutive and reverse consecutive."' failed.

The change fixes this issue by correctly calculating the PreviousInput and the NextInput.
Let me know if this is not the correct approach.

The patch title doesn't seem related (or I don't understand how it's related) to the patch, and the patch description doesn't seem to detail how it relates to the code change.

I guess maybe we have unit testsn for some but not all of these wrappers - might be worth checking/sampling to see what the prevailing trend is (I tend to be over-enthusiastic about testing, but I realize these simple wrappers are pretty uneventful)

(food for thought, not a "this must be answered now" sort of thing, but "Generic atom-graph data structure and algorithms " does sound a bit like the original LLD design, which didn't turn out to be a great fit (at least the way it ended up) for ELF, at least - any ideas on what might be done differently to avoid the same situation here when adding ELF support down the line? (my naive perspective is that the mismatch was around slicing up ELF sections into MachO-like atoms on symbol boundaries was the mismatch - rather than adding support for an atom with multiple symbols (that may not be at the start of the atom) and then having a one atom to one ELF section mapping))

Looks great - thanks!

In D58117#1414613, @kcc wrote:

1. I am not sure what problem does it solve. Everything seems to work.
2. We can now safely remove fuzzer_allocator and rename Vector to just plain std::vector. (we don't need this monstrosity any more since we are using private STL now)

Would it be possible/practical/reasonable to split getOrCreateTypeDIE at 643, before the createAndAddDIE? And at that point do the switch to the appropriate CU (basically as soon as we get the context DIE and can determine which unit is the appropriate one to build the type DIE in)

How about we keep the discussion on the original patch (D57986)? I think we explored the problem space a fair bit there & I'll advocate for the same thing here as I did there: Sorting before emission is likely more efficient than keeping a continuously sorted container, if there's a separate "build" and "iterate" phase you can sort between. At least that's my understanding. std::map and MapVector would both grow memory usage, probably not prohibitively, but a fair bit.

I'll test a couple of related cases (implicit special members and nested classes) here in a moment & see if they have any related bugs.

Looks good - thanks!

Here's a bit of a smaller test that might make it clearer (could add some comments to help explain why each part is present):

Would it be simpler/effective to add "-fuse-ld=gold -Wl,--gdb-index" to your linker flags to enable both these things, rather than having specific CMake support for either/both?

Hmm, I don't seem to have had the troubles you've had - I use CMAKE_EXE_LINKER_FLAGS_DEBUG:STRING=-Wl,--gdb-index and... ah, I don't use (or even have in my CMakeCache.txt any mention of) LLVM_USE_LINKER. Not actually sure where/how my build is choosing to use gold. Perhaps it's just my system default once it's installed.

In D56587#1393750, @dblaikie wrote:

In D56587#1393746, @probinson wrote:

In D56587#1393556, @dblaikie wrote:

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

NVPTX also would need it, because they are stuck on DWARF v2.

Any ideas if NVPTX hit this case? my understanding was that NVPTX has a fairly restrictive set of code or actions that can be used.

I think the point where we lookup the other CU and cross over to it should probably be earlier - imagine if all attributes could potentially be CU-local (eg: we could produce a separate string_offsets section for each CU and so have separate strx indexes for each CU) & we should probably have a solution that would be correct in that situation. Does that make sense?

In D58238#1406518, @wolfgangp wrote:

So this basically prevents the debugger from displaying an inconsistent program state when some of the sideeffects from a line preceding its current stopping point have not been completed yet. Makes sense to me. One minor concern: If the sunk instruction does not get sunk past the next source line, we may create some unneeded location list entries, e.g.:

x = y; x = x + 3; z = z ? z + 3 : b; // all on the same line

If the x + 3 is sunk past the assignment to z (but before any code attributed to the next line is executed) we generate an additional location list entry for x, where there is a small gap in the covered range for x, even though the user is not able to observe x until the next line.
Doesn't look like a big deal, though.

In D58088#1406356, @dsanders wrote:

In D58088#1405089, @glenjofe wrote:

That looks simple enough. I haven't gone through the spec but something as simple as:

namespace llvm {
template<class Ptr>
auto to_address(const Ptr &P) noexcept { return P.operator->(); }
template<class T>
constexpr T* to_address(T* P) noexcept { return P; }
}

seems to cover the majority of it (and definitely the bits I need). It's just the std::pointer_traits<Ptr>::to_address(p) based implementation of the fancy pointer overload that's missing.

If it is named to_address and intended to convert any pointer to a raw pointer then it should be:

namespace llvm {
template<class T>
constexpr T* to_address(T* p) noexcept { return p; }
template<class Ptr>
typename std::pointer_traits<Ptr>::element_type* to_address(const Ptr &p) noexcept { return llvm::to_address(p.operator->()); }
}

Well spotted. I'm about to revert the patch though as I've found a way of doing this without using C++20 or defining any new classes that's good enough (because there's no nullptrs) for the GISel Combiner code that wants to use this

In D58406#1405264, @rtrieu wrote:

Last time, this was brought up, passing strings was deemed too expensive since it would cause a string to be created every time:
http://lists.llvm.org/pipermail/llvm-dev/2018-March/122032.html

There's a difference between getting the pass name and the IR unit name. Pass names are hard-coded string literals which can be cheaply referred to by a StringRef. IR units need to create a new string with the description (except Loop and Region, which don't have interesting descriptions yet.)

In D58088#1404769, @dsanders wrote:

I just noticed a big simplification that doesn't need C++20 features or new classes. This:

template <typename WrappedIteratorT,
          typename T1 = typename std::remove_reference<decltype(**std::declval<WrappedIteratorT>())>::type,
          typename T2 = typename std::add_pointer<T1>::type>
using raw_pointer_iterator = pointer_iterator<pointee_iterator<WrappedIteratorT, T1>, T2>;

is functionally equivalent to the raw_pointer_iterator in this patch. pointee_iterator derefs the unique_ptr, while pointer_iterator produces the raw_pointer. If that version sounds good to you then I think it makes sense to revert the llvm::to_address() patch, switch to this implementation and fold this into the GISel Combiner patch as at that point it's too trivial to warrant a dedicated patch. Does that sound good?

Looks good - thanks!

In D58406#1404198, @fedor.sergeev wrote:

In D58406#1404179, @dblaikie wrote:

Alternatively - how would you feel about callers passing in the strings directly?

return !BisectEnabled || checkPass(P->getPassName(), P->getDescription(U));

The Pass and Unit are only used to retrieve the Pass Name and Description (passing the Unit straight back to the Pass that just passed it in) - so why not pass in those (Pass Name and Description) instead?

You mean e.g. changing

!F.getContext().getOptPassGate().shouldRunPass(this, &F))

into

!F.getContext().getOptPassGate().shouldRunPass(this->getName(), F.getDescription()))

Yep, that would resolve the layering issue w/o requiring opaque pointers.

Btw, in new-pm's pass instrumentation we do pass Pass Name instead of a pointer to pass exactly because of this layering issue.
However, note, that there we still pass IR unit itself (wrapped into llvm::Any) instead of "description"
because instrumentations framework is generic and instrumentation functionality is not predefined.
And yet, specific instrumentations like PrintIR do need IRUnit's description and right now every instrumentation does it on its own.

If we could implement a generic way of querying IRUnit description (w/o introducing a layering issue), it would
be beneficial both for legacy and new pm implementations.

In D58406#1404169, @fedor.sergeev wrote:

The need for generic way of getting textual description for IRUnit is obvious both in legacy and new pass manager.
However in this particular approach I dont like how "const void*" is used to erase specific type of IRUnit.

In new pass manager there was a similar task of erasing the type of IRUnit for pass-instrumentation purposes.
And there we decided to wrap the pointer into llvm::Any.

I'm not sure llvm::Any is the best approach here, but it sounds better than absolute anarchy of const void*.

In D58117#1401919, @serge-sans-paille wrote:

Fedora is currently using gcc-9.0.1, svn revision 268719 (see https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec) which triggers this issue. I'll forward the test case to the gcc team and report here.

I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.

In D58088#1401651, @dsanders wrote:

In D58088#1401583, @dblaikie wrote:

In D58088#1396951, @dsanders wrote:

In D58088#1395226, @paquette wrote:

Hmm, It might be nice to have this as a specialization of pointer_iterator if possible.

I think it might be a source of confusion/frustration to have both a raw_pointer_iterator and a pointer_iterator, where raw_pointer_iterator is specifically used for only unique_ptr.

I can see a way to achieve that (std::to_address()) but it requires C++20. Without that, the closest I've been able to get is a raw_pointer_iterator alias to pointer_iterator that has specializations based on whether a third optional argument is true_type or false_type. The main thing I'm missing to fold the alias in is a way to specify the default value of T in a way that resolves to either pointer_iterator's default type T or raw_pointer_iterators default type T depending on whether *Iter is a std::unique_ptr<X>& or not. I can detect the type easily enough but selecting between the two defaults is proving troublesome as it resolves the true and false cases too early and ends up trying to do int::pointer and failing because int isn't a class type.

Would it be difficult to have LLVM's own version of std::to_address (LLVM has had/does have various type traits implmented in-tree when they aren't available in the standard LLVM's using at the moment) or similar to use here?

That looks simple enough. I haven't gone through the spec but something as simple as:

namespace llvm {
template<class Ptr>
auto to_address(const Ptr &P) noexcept { return P.operator->(); }
template<class T>
constexpr T* to_address(T* P) noexcept { return P; }
}

seems to cover the majority of it (and definitely the bits I need). It's just the std::pointer_traits<Ptr>::to_address(p) based implementation of the fancy pointer overload that's missing.

In D58088#1396951, @dsanders wrote:

In D58088#1395226, @paquette wrote:

Hmm, It might be nice to have this as a specialization of pointer_iterator if possible.

I think it might be a source of confusion/frustration to have both a raw_pointer_iterator and a pointer_iterator, where raw_pointer_iterator is specifically used for only unique_ptr.

I can see a way to achieve that (std::to_address()) but it requires C++20. Without that, the closest I've been able to get is a raw_pointer_iterator alias to pointer_iterator that has specializations based on whether a third optional argument is true_type or false_type. The main thing I'm missing to fold the alias in is a way to specify the default value of T in a way that resolves to either pointer_iterator's default type T or raw_pointer_iterators default type T depending on whether *Iter is a std::unique_ptr<X>& or not. I can detect the type easily enough but selecting between the two defaults is proving troublesome as it resolves the true and false cases too early and ends up trying to do int::pointer and failing because int isn't a class type.

In D58117#1401491, @serge-sans-paille wrote:

In D58117#1401457, @dblaikie wrote:

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

Right! https://godbolt.org/z/c3iMsT proves you right. The part that actually matters seems to be the `construct` member forwarding (why is inheritance not enough in that case?).

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

In D57986#1398271, @vsk wrote:

I think this could roughly double the memory utilization of the writer, which might be problematic because the number of records to write can be high. (@dblaikie did some work on reducing memory usage in this area, he might have hard data about this.)

As the write should only occur once, maybe the better tradeoff would be to sort?

How does this relate to D57940?

This has some relatively extensive changes to some of the llvm tools (like sancov, for instance) that aren't tested - perhaps it'd be better to leave those tools usage unmodified (adding the undef section value as a fix to the API change, but nothing more) with fixits and/or followup patches that implement the improved functionality possible with the new API, along with tests?