[PATCH] D47925: Add debug info for OProfile porifling support

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added a comment.

Thanks for the patch!

I assume you've tested this and it works? It looks correct, but I'm not in a position to set up an OProfile environment to verify it and I don't know the OProfile interface well enough to know without testing.



================
Comment at: lib/ExecutionEngine/OProfileJIT/OProfileJITEventListener.cpp:120
+
+    size_t num_entries = std::distance(Begin, End);
+    static struct debug_line_info* debug_line;
----------------
DILineInfoTable is an alias of llvm::SmallVector, so you can use Lines.size() here.


================
Comment at: lib/ExecutionEngine/OProfileJIT/OProfileJITEventListener.cpp:121
+    size_t num_entries = std::distance(Begin, End);
+    static struct debug_line_info* debug_line;
+    debug_line = (struct debug_line_info * )calloc(num_entries, sizeof(struct debug_line_info));
----------------
Why is this static?


================
Comment at: lib/ExecutionEngine/OProfileJIT/OProfileJITEventListener.cpp:124
+
+    for(DILineInfoTable::iterator It=Begin; It != End; ++It){
+        i = std::distance(Begin,It);
----------------
Also because DILineInfoTable is a SmallVector, you can use a range-based iterator here:

```
for (auto InfoPair : Lines) {
  <fill in debug_line[i]>
  ++i;
}
```
The IntelJITEventListener is old code and I don't think we had range-based iterators when it was last updated.


================
Comment at: lib/ExecutionEngine/OProfileJIT/OProfileJITEventListener.cpp:128
+        debug_line[i].lineno = It->second.Line;
+        SourceFileName = Lines.front().second.FileName;
+        debug_line[i].filename = const_cast<char *>(SourceFileName.c_str());
----------------
I'm not sure this copy is going to do anything useful. It's copying the file name into a string that will be destructed at the end of the out loop iteration. I realize this part of the code is copied directly from the IntelJITEventListener, but it's wrong there too. Maybe we get away with it there because the Intel debug info registration function doesn't need the string pointer to outlive the function call. Maybe that's true with OProfile too. In any event, I can't see a reason to copy the string here.


https://reviews.llvm.org/D47925