rebulid-elftc.sh launch through CMake #4422
Comments
|
It is not clear what you mean (and leaving all the template instructions so the one (!) sentence added is buried doesn't help): are you proposing adding the source code of libelftc into the DR repository and building it as part of every DR build? If so, be explicit: "launch through CMake" does not make that clear. If libelftc were built as part of the DR build, we would probably not use a shell script and use first-class CMake code. But we made a decision back when we added libelftc to not include the sources and include the binaries instead. Please elaborate on the logic behind this proposal. ("debug this script" is not clear: do you mean debugging the rebuild-elft.sh (spelled wrong in the title) script itself? It's a pretty short sequence of commands, with all the arguments right there: that seems a lot easier to understand than CMake code where the final set of flags is not as visible in one place.) |
|
I mean "if binaries are absent, launch this *.sh script directly from CMake instead of abort compilation". |
But the binaries are checked in to the repository, so they will never be absent. |
|
For mingw it's absent for now! So now CMake stuck and aborted. |
|
But the drsyms CMakeLists.txt for mingw will either be WIN32 or UNIX and so it will pick a checked-in binary and try to use it, right? It sounds like this is a proposal to add logic to detect a new build platform, and blindly try to run the UNIX script (even if the new platform is not UNIX) in the hope that A) it will work and B) there are no other issues on this new platform. Right? That script rebuild-elftc.sh is for UNIX and assumes certain things are checked in (bmake, etc.), without looking for them in the normal CMake way. On a new unsupported platform, there's a good chance the script is going to fail: it will not work at all for native Windows, e.g., and is not meant to work on all platforms. |
What is the proposed mechanism to detect a new build platform? If neither |
|
Pseudocode: If we have else (only We should check, is libraries with this architecture exist or not. If not, we will launch this script. I ported bmake to MinGW environment, and bmake compiles libraries in my github mirror repo without issues... Yes, this script will be slightly modified to work on MinGW, but it's SLIGHTLY (use bmake not only in Fedora, etc) |
|
IMHO, if the build process is going to use third-party source files, they should be a git submodule or checked into third_party/, not dynamically checked out inside the build process. It should also not use a shell script which then launches another build generator like make/bmake: it should use CMake control code and be integrated into the regular CMake build system. I.e., either these binaries should be as they are: manually built once and checked in, with the action item here to try to provide a useful fatal error message on an unsupported platform (but again I don't think it's easy to identify such a platform without knowing what it is ahead of time: I would have said before that the MSVC-built libraries would work fine for any Windows platform, including MinGW, and so MinGW is not an unsupported platform for drsyms: and indeed they do work with some symbol fudging as shown in #3852 (comment)), or libeltc should always be locally built from sources in a CMake-normalized and first-class manner as part of the build. Running a shell script from CMake is unusual right there, and to have it run make in some rare hard-to-detect scenario does not seem appealing. Maybe others on the project have other opinions: @johnfxgalea, @AssadHashmi, @abhinav92003? |
|
I agree with @derekbruening. If you are really adamant on dynamically checking out the code, at least try to use CMake's external project commands https://cmake.org/cmake/help/latest/module/ExternalProject.html However, I still recommend @derekbruening approach of using cmake directly to compile checked-in libeltc sources. |
|
OK. So I close this issue, and will launch this script manually through bash. |
|
I would like to re-open this issue, as it also affects packaging. Using pre built binaries is considered very bad practice among various linux distros. Without having a fully reproducible build of the components (and checksums), nobody can verify if the contents are actually valid. Using ExternalProject is an option, but fetching external sources is also discouraged. We could use it together with mirrored (copied in) sources, or a git submodule. Along with the listed binaries, there are more:
|
Agreed.
We wouldn't be packaging up tests though right? |
|
With the toolchain switch in #6534, we also do no longer need the elftoolchain prebuilt binaries for Linux. While they are still in the repo, they can be dropped prior to packaging. @derekbruening is there a reason why they are still included for the Linux targets? |
|
Actually, as noted in #6534, Linux still uses libelftc.a for demangling when template arg expansion is needed, as elfutils does not supply that. The libdwarf.a and libelf.a can be removed. I would assume if we added official *BSD or other unix variant support we would just make those use elfutils and would not want these static libs there either. I will make a PR removing lib{elf,dwarf}.a from all but android, macho, and pecoff. Given that the usage has shrunk down to just demangling, it should be easier to change the build rules to build the elftc demangler from sources and eliminate that checked-in library too as maybe just one file elftc_demangle.c needs to be compiled (haven't analyzed its deps though). |
While drsyms on Linux still uses libelftc.a for demangling when template arg expansion is needed, as elfutils does not supply that, the checked-in elftc libdwarf.a and libelf.a binaries for Linux can be removed. (If we added official *BSD or other UNIX variant support we would just make those use elfutils and would not want these static libs there either.) Issue: #5926, #4422
Another alternative would be to rely on the CXX ABI internals of gcc and LLVM: |
) While drsyms on Linux still uses libelftc.a for demangling when template arg expansion is needed, as elfutils does not supply that, the checked-in elftc libdwarf.a and libelf.a binaries for Linux can be removed. (If we added official *BSD or other UNIX variant support we would just make those use elfutils and would not want these static libs there either.) Issue: #5926, #4422
I believe that pulls in the C++ library which complicates the imports and isolation. |
Add a pointer to any prior users list discussion.
Please use the users list https://groups.google.com/forum/#!forum/DynamoRIO-Users for questions or to propose initial feature ideas if you're not sure there's an existing solution. The users list will reach a wider audience of people who may find the information beneficial. If you have already asked on the users list and the consensus was to file a new issue, please include the URL to the discussion thread here.
Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. E.g., "I'm always frustrated when [...]"
It will be easier to debug this script, if it could be launched via CMake on MinGW-w64. Related to issue #638
Describe the solution you'd like
Describe the desired functionality and its intended usage to give some context for how it would be used.
Do you have any implementation in mind for this feature?
Describe alternatives you've considered
Is there currently a method for implementing this functionality using the existing API and this requested feature will add convenience? Or is there no method for accomplishing the desired task using the current API?
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: