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

[Alex Orlov via Phabricator via llvm-commits]

aorlov added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:92
+    std::string SourceCopy;
+    if (Source.end()[0] != '\0') {
+      SourceCopy = Source.str();
----------------
dblaikie wrote:
> 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)
I just used the same checking as llvm/lib/Support/LineIterator.cpp, line 47:

```
assert(Buffer.getBufferEnd()[0] == '\0');
```
Any suggestions? 


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:305
+      std::string SourceCopy;
+      if (*Source.end() != '\0') {
+        SourceCopy = Source.str();
----------------
dblaikie wrote:
> 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?
Note currently line_iterator does not skip blank lines.
I will prepare an own implementation without llvm::line_iterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102355