[PATCH] D108261: [DebugInfo] Fix end_sequence of debug_line in LTO Object

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee added inline comments.


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:517-518
+    auto &Writer = Assembler.getWriter();
+    if (Writer.isSymbolRefDifferenceFullyResolvedImpl(Assembler, *LastLabel,
+                                                      *EndLabel, false))
+      EndSequence = EndLabel;
----------------
dblaikie wrote:
> kyulee wrote:
> > dblaikie wrote:
> > > kyulee wrote:
> > > > dblaikie wrote:
> > > > > dblaikie wrote:
> > > > > > When is this condition false? (is it tested?)
> > > > > Still curious about this question ^
> > > > I've attempted to delete this symbol diff check in https://reviews.llvm.org/D108261?id=367123 but
> > > > https://buildkite.com/llvm-project/premerge-checks/builds/53063#d29a2537-c103-4035-86a0-4e01e0188dd4 showed many builds failed like
> > > > ```
> > > > Assertion failed: Abs && "We created a line delta with an invalid expression" 
> > > > ```
> > > > So, I restored this logic back.
> > > > 
> > > > 
> > > > As for `InSet = false`, I think it confirms the absolute symbol difference, or this check can be true optimistically by https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/llvm/lib/MC/MachObjectWriter.cpp#L680-L681.
> > > > I tested the following test could fail if I set `InSet = true`.
> > > > ```
> > > > DebugInfo/X86/gmlt.test
> > > > ```
> > > > 
> > > This seems suspicious to me - "isSymbolRefDifferenceFullyResolvedImpl" (also a bit suspicious that this is calling an "Impl" function - usually those are only used as implementation details for some non-impl function, where the latter is intended as the general entry point) should only return false if the two symbols aren't in the same section. I wouldn't've thought that should happen - when/where/how/why does that happen?
> > I've just replaced `isSymbolRefDifferenceFullyResolvedImpl` by `isSymbolRefDifferenceFullyResolved`. 
> > As shown https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/llvm/include/llvm/MC/MCObjectWriter.h#L67-L86, the arguments are very similar in these two except that one is with `MCSymbolRefExpr` and the other is with `MCSymbol`.
> > 
> > Are you suggesting a new API `isSymbolRefDifferenceFullyResolved(constMCAssembler &Asm,   constMCSymbolRefExpr *A,   constMCSymbolRefExpr *B)` that defaults with `InSet = false`, and use it here?
> Sorry, no I don't mean to rename or refactor this API.
> 
> I'd like to understand why this function call ever produces a result of `false` - it seems to me that EndLabel and LastLabel should always be from the same section, and if they are, I think this function should never return false - so I think I'm misunderstanding something/don't understand where these labels are coming from such that they could end up being from different sections. I'd like to understand how that situation can arise before approving this patch - to understand better why this approach/test is suitable.
I'm not familiar with the whole logic, but I've traced a particular case below.
```
DebugInfo/X86/cu-ranges.ll
```
This test runs with `-function-sections` so it appears each function goes to each section (comdat?).
I found `LastLabel` and `EndLabel` are in the same section for the last function only because `EndLabel` is set from the last function.
This means all the prior functions go with the end section label because this symbol difference check fails.
For the last function, `EndLabel` is actually the same offset to the end section label because each section has its own function only.
So, I think this logic seems to cover this case correctly resulting in a valid line table. I'm not sure if there is a better way to guard this case.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108261/new/

https://reviews.llvm.org/D108261