[Lldb-commits] [PATCH] D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D59854#1443915 <https://reviews.llvm.org/D59854#1443915>, @zturner wrote:

> In D59854#1443906 <https://reviews.llvm.org/D59854#1443906>, @jingham wrote:
>
> > In D59854#1443904 <https://reviews.llvm.org/D59854#1443904>, @JDevlieghere wrote:
> >
> > > In D59854#1443891 <https://reviews.llvm.org/D59854#1443891>, @jingham wrote:
> > >
> > > > It isn't safe to pass "data()" of a StringRef to dlsym.
> > >
> > >
> > > Why?
> >
> >
> > StringRef's aren't guaranteed to be NULL terminated.  Or rather, the Data that gets returned by the data() method is just the raw pointer in the StringRef, which isn't guaranteed to be NULL terminated.
>
>
> While true, I don't think this is a good reason to make the interface accept a `const char*`, because that means that the interface is exposing an implementation detail.  Instead, the interface should accept a `StringRef`, and the implementation can pass `foo.str().c_str()`.  It might be slightly less efficient (almost to the point of minutia), but this isn't really performance sensitive code, as loading the library will dominate the call to malloc / memcpy.


I don't actually care one way or the other about using a StringRef here (though I still think it is odd to have the library be a char * and the symbol a StringRef...).  I was only concerned that you can't pass the data() of a StringRef to anything that expects a c-string.


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

https://reviews.llvm.org/D59854