[PATCH] D38830: [DWARF] Fix bad comparator in sortGlobalExprs.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

Eli, I think this looks good.

As far as I know this method is focusing on sorting (and removing duplicates) for the fragmented expressions, so we just need to get all expressions without fragment info (including the null exprs) out of the way to let std:unique do its thing. We aren't guaranteed to remove all duplicates for the expressions without fragment info anyway (due to only creating a strict weak ordering for the expressions with fragment info before std::unique).
So in our out-of-tree target workaround we currently make no difference between the null exprs and non-fragmented expressions, but your solution should work as well.

I also think that creating a test case might be troublesome. Mikael Holmén had some reproducer for this, but I doubt that it still works.
Maybe it can be tested by creating some unit test, but I guess that would be "little bang for the buck".



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:521
             });
   GVEs.erase(std::unique(GVEs.begin(), GVEs.end(),
                          [](DwarfCompileUnit::GlobalExpr A,
----------------
We should probably highlight here (or in the function header) that the uniqueness only is "guaranteed" for the expressions containing fragment offsets.

Afaik, std::unique is only removing consecutive elements that are equal. And the expressions without fragment info are not really sorted (they all end up before the fragmented expressions, but the non-fragmented expressions are not ordered relative to each other).
If we really want to remove all duplicates, then I think we need to use a more complex algorithm for removing duplicates instead of using std::unique.


Repository:
  rL LLVM

https://reviews.llvm.org/D38830