[Lldb-commits] [PATCH] D53759: [PDB] Support PDB-backed expressions evaluation

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov marked 2 inline comments as done.
aleksandr.urakov added inline comments.


================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:819
   if (!section_list)
     m_entry_point_address.SetOffset(offset);
   else
----------------
Hui wrote:
> This still needs to be the offset into the section.
If we are here, then we can't retrieve a section list, then we can't specify a section offset here. Setting an offset without setting a section means that offset must be treated as a file address (see e.g. the implementation of the `Address::GetFileAddress()` function). I think that misleading here is the name of the variable `offset`, I'll fix it, thanks.


================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindows.cpp:959
+    // It may be an expression evaluation crash.
+    SetPrivateState(eStateStopped);
   }
----------------
stella.stamenova wrote:
> Are we going to determine later whether it is supposed to be a crash or just never crash on second chance exceptions?
Yes, it's a good question... I can't understand how to figure out at this point if the exception was thrown during an expression evaluation or a debuggee execution. On other hand, it seems that Visual Studio also doesn't crash an application in such situations:
```
int main() {
  char *buf = nullptr;
  buf[0] = 0; // Unhandled exception is here, but you can safely continue an execution if you will drag an arrow on the next line in Visual Studio
  int x = 5;
  return x - 5;
}
```
Also it was the only place where `eStateCrashed` was set, so it seems that on other platforms we do not crash a debuggee in such situations too. So may be it's not a bad idea to not set the crashed state here at all. But if someone don't agree with me and has an idea how to do it better, I'm ready to fix it.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:1085
+  } else
+    set = &m_namespaces;
+  assert(set);
----------------
stella.stamenova wrote:
> Nit: Maybe add curly braces here. Since the if statement has them, the code is easier to read if the else does as well.
Sure, thanks!


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53759