SBCommandReturnObject: change LLDB_RECORD_METHOD(..., FILE *, ...) to use LLDB_RECORD_DUMMY

[MaskRay]

Summary

POSIX says FILE is a typedef to a structure containing information about
a file. The structure is unspecified, i.e. it may be an incomplete type, as is the case on musl
(struct _IO_FILE is an implementation detail that is not exposed).

LLDB_RECORD_METHOD(..., (FILE *), ...) transitively uses sizeof(FILE)
and requires the structure to be complete. Change it to
LLDB_RECORD_DUMMY to fix the build failure on musl (regression of
D57475).

[MaskRay]

This fixes https://pastebin.com/vMZMFWh3

[ 81%] Building CXX object source/API/CMakeFiles/liblldb.dir/SBCommandReturnObject.cpp.o
In file included from /src/vanilla/dev/lldb/lldb-9.0.0.src/source/API/SBCommandReturnObject.cpp:10:
In file included from /src/vanilla/dev/lldb/lldb-9.0.0.src/source/API/SBReproducerPrivate.h:18:
/src/vanilla/dev/lldb/lldb-9.0.0.src/include/lldb/Utility/ReproducerInstrumentation.h:567:58: error: invalid application of 'sizeof'
      to an incomplete type '_IO_FILE'
      m_stream.write(reinterpret_cast<const char *>(&t), sizeof(T));

I am not clear what this file does but deleting these methods pass the tests (LD_LIBRARY_PATH=~/llvm/Release/lib ninja -C ~/llvm/Release check-lldb).

[labath]

The test suite passes because this was (just now) changed into a deprecated api, but we still want to keep that for backward compatibility. The main problem here is the reproducers being weird and wanting to make copies of types they can't possibly understand.

I have a feeling this problem has appeared before, but I don't recall what was done to fix it. Jonas, do you remeber that by any chance?

[MaskRay]

In D68872#1706003, @labath wrote:

The test suite passes because this was (just now) changed into a deprecated api, but we still want to keep that for backward compatibility. The main problem here is the reproducers being weird and wanting to make copies of types they can't possibly understand.

I have a feeling this problem has appeared before, but I don't recall what was done to fix it. Jonas, do you remeber that by any chance?

Shall I just delete LLDB_RECORD_METHOD(..., (FILE *), ...); but keep these definitions? Does that keep the compatibility?

[labath]

In D68872#1706006, @MaskRay wrote:

In D68872#1706003, @labath wrote:

The test suite passes because this was (just now) changed into a deprecated api, but we still want to keep that for backward compatibility. The main problem here is the reproducers being weird and wanting to make copies of types they can't possibly understand.

I have a feeling this problem has appeared before, but I don't recall what was done to fix it. Jonas, do you remeber that by any chance?

Shall I just delete LLDB_RECORD_METHOD(..., (FILE *), ...); but keep these definitions? Does that keep the compatibility?

Probably you ought to replace them with LLDB_RECORD_DUMMY, but I'm not 100% sure of that. Let's wait to see what Jonas has to say here...

[lawrence_danna]

I don't think we can just delete these. They're API that existing C++ clients might still be using.

I'm pretty close to having all the FILE* stuff out of the SIWG interface however. When that's done I'll delete the FILE* typemaps and anything with a FILE* in it will be only accessible by C++ clients. Python clients that call the old FILE* APIs will be getting the overloaded FileSP version instead.

[MaskRay]

In D68872#1708602, @lawrence_danna wrote:

I don't think we can just delete these. They're API that existing C++ clients might still be using.

I'm pretty close to having all the FILE* stuff out of the SIWG interface however. When that's done I'll delete the FILE* typemaps and anything with a FILE* in it will be only accessible by C++ clients. Python clients that call the old FILE* APIs will be getting the overloaded FileSP version instead.

Can you provide a replacement?

[lawrence_danna]

In D68872#1710331, @MaskRay wrote:

Can you provide a replacement?

What do you mean?

If you mean a replacement for the FILE* methods for python clients, that is in place. The FileSP overloads will behave the same as the FILE* ones did from python's point of view, except with well-defined and non-leaky ownership semantics, and with support for arbitrary subclasses of IOBase.

If you mean for C++ clients, there's no need for a replacement, because the C++ methods are all still there. They're just not accessible from python. The FILE* APIs are already safe to use from C++, so those clients shouldn't need to change.

[MaskRay]

In D68872#1710334, @lawrence_danna wrote:

In D68872#1710331, @MaskRay wrote:

Can you provide a replacement?

What do you mean?

If you mean a replacement for the FILE* methods for python clients, that is in place. The FileSP overloads will behave the same as the FILE* ones did from python's point of view, except with well-defined and non-leaky ownership semantics, and with support for arbitrary subclasses of IOBase.

If you mean for C++ clients, there's no need for a replacement, because the C++ methods are all still there. They're just not accessible from python. The FILE* APIs are already safe to use from C++, so those clients shouldn't need to change.

I want to fix the reliance on sizeof(FILE). The underlying type of FILE is unspecified in POSIX and indeed this case on linux-musl - so lldb 9.0.0 and trunk currently fail to build on linux-musl. I know very little about the code, and I hope you or @JDevlieghere can suggest a way forward.

[lawrence_danna]

In D68872#1710366, @MaskRay wrote:

I want to fix the reliance on sizeof(FILE). The underlying type of FILE is unspecified in POSIX and indeed this case on linux-musl - so lldb 9.0.0 and trunk currently fail to build on linux-musl. I know very little about the code, and I hope you or @JDevlieghere can suggest a way forward.

Just use LLDB_RECORD_DUMMY. Look in SBFile.cpp for an example. The reproducer instrumentation cannot actually make use of FILE* arguments at this point anyway.

(make sure to run the reproducer tests to confirm that it doesn't break it)

[MaskRay]

Use LLDB_RECORD_DUMMY

[MaskRay]

Delete redundant newlines

[MaskRay]

In D68872#1710371, @lawrence_danna wrote:

In D68872#1710366, @MaskRay wrote:

I want to fix the reliance on sizeof(FILE). The underlying type of FILE is unspecified in POSIX and indeed this case on linux-musl - so lldb 9.0.0 and trunk currently fail to build on linux-musl. I know very little about the code, and I hope you or @JDevlieghere can suggest a way forward.

Just use LLDB_RECORD_DUMMY. Look in SBFile.cpp for an example. The reproducer instrumentation cannot actually make use of FILE* arguments at this point anyway.

(make sure to run the reproducer tests to confirm that it doesn't break it)

Thanks for the suggestion. Applied. I have verified LD_LIBRARY_PATH=~/llvm/Release/lib ninja -C ~/llvm/Release check-lldb passes.

(I need LD_LIBRARY_PATH= to make check-lldb-lit pass on Linux. lit tests load ~/llvm/Release/lib/python2.7/dist-packages/lldb/_lldb.so, which specifies DT_RUNPATH=$ORIGIN/../lib. Unfortunately $ORIGIN is relative to the symlink itself, not the target file ~/llvm/Release/lib/liblldb.so.10.0.0svn)

Did you suggest ninja check-lldb-shell-reproducer? It passes as well.

[labath]

In D68872#1710418, @MaskRay wrote:

Thanks for the suggestion. Applied. I have verified LD_LIBRARY_PATH=~/llvm/Release/lib ninja -C ~/llvm/Release check-lldb passes.

(I need LD_LIBRARY_PATH= to make check-lldb-lit pass on Linux. lit tests load ~/llvm/Release/lib/python2.7/dist-packages/lldb/_lldb.so, which specifies DT_RUNPATH=$ORIGIN/../lib. Unfortunately $ORIGIN is relative to the symlink itself, not the target file ~/llvm/Release/lib/liblldb.so.10.0.0svn)

Is this a regular or some kind of a shared library build? @hhb, is this expected?

[JDevlieghere]

Somehow I missed this in my review queue.

Larry is correct, the reproducers don't do anything meaningful with a FILE*. If these methods matter to the reproducer (something I haven't spent time investigating yet) then we have to provide a custom solution, like we did for SBDebugger. There we "shadow" the file and swap out the FILE* during replay.

TL;DR: Adding a DUMMY Is fine as long as these these methods don't matter to the reproducer, which appears to be the case.

lldb/source/API/SBCommandReturnObject.cpp

 
 size_t SBCommandReturnObject::GetErrorSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetErrorSize);
 
   return ref().GetErrorData().size();
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
-  LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
+  LLDB_RECORD_DUMMY(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
   if (fh) {
     size_t num_bytes = GetOutputSize();
     if (num_bytes)
       return ::fprintf(fh, "%s", GetOutput());
   }
   return 0;
 }
 
 size_t SBCommandReturnObject::PutOutput(FileSP file_sp) {
   LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (FileSP),
                      file_sp);
   if (!file_sp)
     return 0;
   return file_sp->Printf("%s", GetOutput());
 }
 
 size_t SBCommandReturnObject::PutOutput(SBFile file) {
   LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (SBFile), file);
   if (!file.m_opaque_sp)
     return 0;
   return file.m_opaque_sp->Printf("%s", GetOutput());
 }
 
 size_t SBCommandReturnObject::PutError(FILE *fh) {
-  LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutError, (FILE *), fh);
-
+  LLDB_RECORD_DUMMY(size_t, SBCommandReturnObject, PutError, (FILE *), fh);
   if (fh) {
     size_t num_bytes = GetErrorSize();
     if (num_bytes)
       return ::fprintf(fh, "%s", GetError());
   }
   return 0;
 }
 
bool SBCommandReturnObject::GetDescription(SBStream &description) {
 
   if (GetErrorSize() > 0)
     strm.Printf("\nError Message:\n%s", GetError());
 
   return true;
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
-                     (FILE *), fh);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateOutputFile,
 
   SetImmediateOutputFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
-                     (FILE *), fh);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateErrorFile,
 
   SetImmediateErrorFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh,
                                                    bool transfer_ownership) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
-                     (FILE *, bool), fh, transfer_ownership);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateOutputFile,
   FileSP file = std::make_shared<NativeFile>(fh, transfer_ownership);
   ref().SetImmediateOutputFile(file);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh,
                                                   bool transfer_ownership) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
-                     (FILE *, bool), fh, transfer_ownership);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateErrorFile,
   FileSP file = std::make_shared<NativeFile>(fh, transfer_ownership);
   ref().SetImmediateErrorFile(file);
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(SBFile file) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
                      (SBFile), file);
   ref().SetImmediateOutputFile(file.m_opaque_sp);