[PATCH] D102355: Add support for DWARF embedded source to llvm-symbolizer

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:92
+    std::string SourceCopy;
+    if (Source.end()[0] != '\0') {
+      SourceCopy = Source.str();
----------------
This still isn't valid - there's no guarantee that end is dereferenceable. (using `[]` doesn't make the code any safer/different - it's still a dereference)


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:305
+      std::string SourceCopy;
+      if (*Source.end() != '\0') {
+        SourceCopy = Source.str();
----------------
aorlov wrote:
> aorlov wrote:
> > dblaikie wrote:
> > > It's not valid to dereference an end iterator like this.
> > > 
> > > What is this code intended to address?
> > Note llvm::line_iterator is badly designed. 
> > It is checks *Pos == '\0' in many places instead of comparing Pos with Buffer.end().
> > It requires '\0' at the end of buffer which is not necessary actually.
> > Look at an assert on llvm/lib/Support/LineIterator.cpp, line 47.
> Note the current implementation of `line_iterator` can be used with the full source, but not with the pruned source if it didn't coincide with the end of the source file.
Either the code will need to unconditionally copy the StringRef into a std::string (in which case, maybe it'd be simpler if the SourceCode structure stored the data in a std::string in the first place? then it wouldn't need the MemoryBuffer member either, since it wouldn't be holding live pointers to the memory mapped file) - or the line_iterator would need to be fixed or a different tool used - which, maybe that's the thing to do? If the original code that extracted the relevant lines already used the line iterator (so features like blank line skipping, etc, have already been handled) - maybe this code could do something simpler, like splitting on newlines, rather than using the full line iterator?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102355