- User Since
- Dec 9 2012, 11:41 PM (382 w, 2 d)
I think that a clang-pcm like tool would be incredible. Working with modules is somewhat frustrating because there is no good supported way to see what the module structure that clang actually sees. But that goes well beyond an attempt to repair the NetBSD builds.
@jhenderson Traditionally, the long options all must come after the short options. Doing a prefix match seems reasonable way to handle this. Honestly, all the arguments have been "we like the GNU options because that's what we are used to" rather than any real technical issue. I don't think that is a valid justification for breaking existing users of these options.
@dexonsmith - yeah, sadly I dont think that there is a good way to audit that - any change to the public headers can cause issues. Furthermore, the libc headers themselves also influence this.
@ldionne - "hard coding" is the right solution. If the user specifies a value on the command line or through a cache file, that will be the value used. What I suggested works as a default value.
The tests that I was requesting were for clang/LLVM. Testing libunwind is challenging as it requires runtime tests, and that means that in order to test it effectively, you need each type of machine. I suppose that in theory, you could build a VM based on LLVM's JIT, cross-compile the library, binary translate the code and test it - but that seems incredibly fragile.
Mon, Apr 6
Fri, Apr 3
Im surprised that no tests need changing here. Could you please add a clang/llvm test to validate that the generated information matches what libunwind will start looking for?
@ldionne yeah, thats why I would prefer that we just do the following in the top level CMakeLists.txt for libunwind:
Okay, thanks to some help from @JDevlieghere I was able to get a test going for this. I think that the basic test is sufficient for this. I think that the path sanitizing and conversion should be a subsequent change.
I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.
Thu, Apr 2
The precedent for behaviour based on the name is long standing and not being introduced here. In fact the archiver already does this. clang also does this (clang vs clang++). This is not at all uncommon I think. The target triple prefixed tools should be treated as the tool that they specify and implicitly passing -target ... which is derived from the prefix.
I think that @ldionne should chime in on this change. Mechanically, sure, I don't see any problems with the change.
Thanks for the hint about the string conversion, however, I think that it's going to complicate things as the string is going to be a mixture of UTF-8 and UTF-16 content.
Wed, Apr 1
This should be safe, if pc is ever 0, we don't have any chance of recovery.
@jhenderson - accepting GNU options in GNU mode makes sense. This tool is *not* GNU - objdump is the GNU driver, llvm-objdump is the LLVM driver and accepts SUN style arguments.
Seems reasonable, though this isn't UB, its just use of an uninitialized variable.
Sat, Mar 28
Please address the linter warnings. I think that adding an additional test to demonstrate that the experimental-ifs-version-v1 is properly diagnosed is a good idea. LGTM other than that.
Tue, Mar 10
*sigh* ELF, couldn't you just add that macro to the standard? (We have something similar in libunwind to deal with this). LGTM
Mon, Mar 9
LGTM, and tested with Swift as well that it does resolve the issue. Thanks for fixing this!
Mar 5 2020
Feb 27 2020
Feb 19 2020
@smeenai I agree that -lz is less than idea, however, for llvm-config, I think that is fine. In general, I think that we will want to phase out llvm-config in the longer term in favor of pkgconfig and CMake.
Feb 12 2020
Sure, seems reasonable.
Feb 10 2020
Feb 7 2020
Feb 5 2020
I think that we should switch everything to just _Unwind_Personality_Fn and remove __personality_routine.
Ugh, I forgot about one thing. Could you please ensure that the path is still embedded into the output of llvm-config --libs?
Linking against the imported target should do the right thing and add interface headers too.
Feb 2 2020
Already committed in a43bf8078861ae816b45d94f7012a7ea01a2e5eb
I think that we would want to retain the ability to build libc++abi against libgcc_s. If that also vends a __personality_routine type, then this would definitely be preferable.
Feb 1 2020
Jan 31 2020
Why not rename __personality_routine to _Unwind_Personality_Fn? I don't think that _Unwind_Personality_Fn is part of either the ABI-EH, gABI, or PSABI, but, it is acceptable for GCC compatibility.
Jan 28 2020
Jan 27 2020
Thanks for fixing this
Jan 23 2020
I think that removing the nested layout is something which should be removed separately from this change. That would move us closer to declaring the nested layout as being unsupported. While I think that the move towards that makes sense, it should be brought up on llvm-dev first I think.
Jan 20 2020
Do we need to worry about ordering of the plugins?
Jan 13 2020
Do we need to worry about compatibility? What happens if there is a mix between an old binary and a new binary?
I'm not sure I understand the motivation for this change. Can you please expand on that? How are you using it that it doesn't work for you? Additionally, z is definitely the wrong name, please compute that by using get_file_name_component, CMAKE_SHARED_LIBRARY_PREFIX, and string(REGEX...)
Jan 7 2020
Jan 2 2020
Jan 1 2020
Hmm, I see, yes, that would change the behavior where we would not create the targets for it. In that case, using EXCLUDE_FROM_ALL is fine, but set it as a target property rather than passing global variables like this. But, even better would be to understand if we even need this.
Dec 24 2019
Dec 23 2019
Dec 22 2019
Dec 16 2019
Dec 12 2019
Dec 10 2019
Should probably add a check for __block variables.
Dec 9 2019
The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.
Dec 7 2019
Dec 6 2019
Dec 4 2019
LGTM with the additional test for the size/type mismatch validation.
Dec 3 2019
There isn't a workaround for this, its just a bug in our CMake. I've fixed the issue with our CMake logic in GIT 372ad32734ecb455f9fb4d0601229ca2dfc78b66.