[PATCH] D44562: [ELF] Rework debug line parsing to use llvm::Error and callbacks (LLD-side)

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: ELF/InputFiles.cpp:128
+    // for compilation unit (CU) of interest. We have only one
+    // CU (object file), so offset is always 0.
+    Expected<const DWARFDebugLine::LineTable *> ErrOrLineTable =
----------------
grimar wrote:
> I would move this comment above `isValidOffset(0)` and update it as you are
> doing something with offset 0 right there already.
Actually, the comment (and code) is technically wrong - it is possible for there to be multiple CUs in a single object input. I'm thinking objects built via LTO or -r links. I'll add a FIXME and file a bug, along with updating the comment.


================
Comment at: ELF/InputFiles.cpp:136
+                      [&](DebugLineError &Err) { warn(Err.message()); },
+                      [&](ErrorInfoBase &Err) { error(Err.message()); });
+  }
----------------
grimar wrote:
> Is it useful to `warn` here? I think we only call this method on linkage error,
> so anyways going to terminate the link.
> With that, it seems easier to always error out here to simplify code/logic.
I'm currently preserving behaviour (or fixing intended behaviour, at least), though I certainly see where you're coming from. In general we try to emit as many errors/warnings as we can before stopping, so I still think it's useful - it might help the user identify some other problem, for example.

I'm not bothered either way though, so if the consensus is to not emit the warning, I can easily change that.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44562