Page MenuHomePhabricator

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

Authored by MaskRay on Oct 11 2019, 8:13 AM.

Details

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).

Diff Detail

Event Timeline

MaskRay created this revision.Oct 11 2019, 8:13 AM

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).

MaskRay edited the summary of this revision. (Show Details)Oct 11 2019, 8:16 AM
labath requested changes to this revision.Oct 11 2019, 8:19 AM

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?

This revision now requires changes to proceed.Oct 11 2019, 8:19 AM
MaskRay edited the summary of this revision. (Show Details)Oct 11 2019, 8:20 AM

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?

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...

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.

lawrence_danna requested changes to this revision.Oct 14 2019, 3:24 PM

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 added a comment.EditedOct 15 2019, 6:52 PM

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.

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 added a comment.EditedOct 15 2019, 8:03 PM

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 updated this revision to Diff 225154.Oct 15 2019, 9:30 PM
MaskRay retitled this revision from SBCommandReturnObject: delete functions that take FILE* as arguments to SBCommandReturnObject: change LLDB_RECORD_METHOD(..., FILE *, ...) to use LLDB_RECORD_DUMMY.
MaskRay edited the summary of this revision. (Show Details)

Use LLDB_RECORD_DUMMY

MaskRay updated this revision to Diff 225155.Oct 15 2019, 9:32 PM

Delete redundant newlines

Harbormaster completed remote builds in B39619: Diff 225155.
MaskRay added a comment.EditedOct 15 2019, 9:41 PM

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 accepted this revision.Oct 16 2019, 3:24 AM
labath added a subscriber: hhb.

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?

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.

JDevlieghere accepted this revision.Oct 16 2019, 10:25 AM
lawrence_danna accepted this revision.Oct 16 2019, 3:10 PM
This revision is now accepted and ready to land.Oct 16 2019, 3:10 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2019, 6:27 PM