[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: tools/lldb-vscode/SourceBreakpoint.h:24
+  // Set this breakpoint in LLDB as a new breakpoint
+  void SetBreakpoint(const char *source_path);
+};
----------------
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > zturner wrote:
> > > > `StringRef source_path`
> > > I am gonna leave this one as is because we must pass a "const char *" the API that is called inside the body of this method:
> > > 
> > > ```
> > >   lldb::SBBreakpoint lldb::SBTarget::BreakpointCreateByLocation(const char *file, uint32_t line);
> > > ```
> > > 
> > In that case you can use `const llvm::Twine &`.  You can pass many types of objects to it, such as `const char*`, `std::string`, `llvm::StringRef`.  Then, inside the function you can write:
> > 
> > ```
> > llvm::SmallString<32> S;
> > StringRef NTS = source_path.toNullTerminatedStringRef(S);
> > ```
> > 
> > If the original parameter was constructed with a `const char*` or `std::string`, then `toNullTerminatedStringRef` is smart enough to know that it doesn't need to make a copy, and it just returns the pointer.  If it was constructed with a `StringRef`, then it null terminates it using `S` as the backing storage and returns that pointer.  So it's the best of both worlds
> That is just way too much work for no gain.
The gain is that `const char*` is unsafe and error prone and should be avoided wherever possible unless it’s very cumbersome not to.

The reason is that it's a bit like `const` propagation.  It's inflexible and as soon as one function parameter is `const`, it is restricted to only being able to call other `const` functions, and it trickles downward throughout the entire program.  Now in this case it's good, because `const` is intended to help you, but in the cast of raw string pointers like `const char *` it's bad, because raw pointers in general are unsafe and error prone, and in this case additionally inefficient (if multiple functions need to do something with the string, you end up with multiple calls to `strlen`).

It's true that the implementation of the function needs to call an SB API function, but interface design and interface implementation should be orthogonal.  You pass a `StringRef` because it's the right thing to do for the interface, that the implementation needs a null terminated string is an implementation detail (some day, for example, we might have an SB API v2 that can be made to work with C++ types via clever SWIG mappings or some other technology).

It might be a minor nitpick, but we have a basically clean slate here so I would prefer to start on the right foot with modern c++ constructs and paradigms


================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:2641-2645
+        // We must open two FILE objects, one for reading and one for writing
+        // the FILE objects have a mutex in them that won't allow reading and
+        // writing to the socket stream.
+        g_vsc.in = fdopen(socket_fd, "r");
+        g_vsc.out = fdopen(socket_fd, "w");
----------------
Now I see what you mean about `FILE*` being able to be used with a port.  But I wasn't familiar with this behavior of `FILE*`.  My undersatnding of `FILE*` is that its behavior is implementation defined.  So can we guarantee this mutual exclusion on all platforms?  I don't even know Windows' behavior with respect to this, I will have to check on this first to see if Windows behaves the same way.

If I understand your comment correctly though, it sounds like you're saying that you can't simultaneously read from and write to a full duplex socket, so the main advantage to using `FILE*` is that it gives us a mutex?  How does it know to use the same mutex for both `in` and `out`?  It seems like it would only guarantee mutual exclusion per-file object.  So you couldn't read from the same socket on multiple threads, and you couldn't write to the same socket on multiple threads.  But the current code still allows reading from and writing to the same socket simultaneously.  Is that ok?

I need to confirm this behavior on other platforms.  That said, we could also just put a mutex in the global `VSCode` class.


https://reviews.llvm.org/D50365