- User Since
- Dec 9 2012, 11:41 PM (435 w, 2 d)
Thu, Apr 1
Wed, Mar 31
Tue, Mar 30
Mar 13 2021
I don't think that LC_FUNCTION_STARTS would create an actual section.
Mar 9 2021
Mar 8 2021
Im afraid I dont understand the purpose of this change. libunwind isn't meant to be merged into libc++/libc++abi. The reasoning for libc++ to support merging libc++abi doesn't really apply to libunwind IMO. Can you please elaborate what exactly the reasoning for this is?
Mar 2 2021
Thanks for the updates!
Ah right, the !RISCV -> RISCV case. This makes sense to commit to fix the immediate issue.
Mar 1 2021
This is fine for now, but in the future, for such changes, please split this up into a series of patches. The clean ups for the named constants and clean up for floating point handling could have been separate changes which would have reduced the overall size of the changes and focused the patch specifically to adding support for rv32.
Feb 22 2021
Feb 10 2021
I suppose that this makes it difficult to generate them when libunwind is built standalone, but, as it is, there are other dependencies involved.
Jan 29 2021
+1 to @int3's suggestion about the help text.
Jan 25 2021
Did I miss the test that would cover the deserialization of bitcode from a -force_load'ed archive?
I'm really not sure what ObjCARC maps to. ObjCARCOpts is the correct component name.
Jan 21 2021
Sure, but if the reason for the group is to add structure to the help output, I think that is a reasonable thing to state in the commit message. The other options are ... well, either not meant to be part of the commit as per the commit message or the commit message should be updated to indicate that they are being added (and why they are part of structuring the help output, assuming that is the reason for the group). The changes requested are requests for clarification :)
I can sympathize with the desire to avoid the undecoration. Unless @ldionne has some reason why this is a poor choice to support, this seems okay to me.
Looks reasonable enough to me. Please ensure that @ldionne is okay with it as well.
Jan 20 2021
I like the clean up here, but, I think there is some value in retaining the defensive sanity checks. You are correct that in most cases, this should never be hit in a proper unwind, but because this may be run in a last chance scenario, having the defensive checks is good for catching things going wrong.
Seems like this just adds an option to control a hardcoded value, which is reasonable.
Perhaps @rnk has any thoughts, but this seems very straightforward.
Jan 19 2021
Welcome back, hope you had a good time off. Thanks for the review!
Could you please add a test case for verifying the output order of the load commands? It seems that the load commands are expected to be in a certain order according to your change - it makes sense to have a test which validates that ordering.
This adds additional options while also adding a group for the versions. Is there a reason for the group to be added? Can you please either remove the newly added options or adjust the commit message? I think that there should be some reasoning given for the version group creation (at least, it isn't clear to me why the group is helpful/necessary, unlike in clang where the group can be used to silence warnings).
Jan 18 2021
Ping x 3
Jan 14 2021
I agree with Renato, this seems incorrect. In the EABI mode, we should prefer the EABI symbols.
Jan 13 2021
Jan 12 2021
You mean use armv7---elf as a triple? While that is possible, keep in mind that it might change the ABI of the generated interfaces. While it is unlikely to matter as the ifso cannot be used at runtime, if the linker attempts to do something like symbolic resolution of ABI dependent specialization (e.g. strcmp.eabi vs strcmp where the latter is generic), that could matter. However, adding support to explicitly override the encoded triple based on the user's input is reasonable (though might deserve a warning that the triple is being overridden).
The full triple is needed in some cases: armv7-unknown-linux-gnueabi vs armv7-unknown-linux-gnueabihf vs armv7-unknown-linux-eabihf vs armv7-unknown-linux-eabi are all different. Additionally, there is nothing that prevents you from doing something like: armv7-unknown-linux-gnueabihf-coff or armv7-unknown-linux-gnueabihf-macho to generate COFF or MachO object files that are reading in additional bits of ABI information and the libc from the target triple. I really think that preserving the target triple is best.
I think that it makes more sense to ignore the aapcs-vfp calling convention attribute (returning CCCR_Ignore), and warn on the aapcs. The redundant calling convention attribute does nothing, and I think that simply ignoring it would solve your issue. However, changing the calling convention to aapcs would break interoperability, so warning on that seems reasonable still. I think that if Wine is willing to adjust their use of the attribute, that would really be nicer - it allows us to keep the behaviour as close to MSVC as possible.
Ping x 2
Jan 11 2021
Can you please add some more context around the motivation for this change? It seems ... unnecessary as everything is already AAPCS/AAPCS-VFP.
LGTM; please fix the formatting lint.
Jan 8 2021
Jan 7 2021
Jan 5 2021
Gentle post-holiday ping :)
Dec 22 2020
The change on L722 took me a few moments to catch - that was subtle.
Dec 21 2020
Dec 17 2020
I really would prefer the alternate spelling, but the change itself is okay.
Dec 16 2020
Dec 14 2020
Dec 10 2020
Hmm, I think that the function is marked as noexcept, so the the funclet shouldn't be invoked and should get DCE'd, but that seems unrelated to ARC.
Dec 9 2020
There is nothing particularly special about that. The reason for the funclet handling there is that in the case of an outlined block for exception handling (i.e. a funclet), we need to ensure that the assembly marker received the funclet token as failure to do so would mark the assembly as unreachable and would thus thwart the auto-release claim. For the normal codepath, the behaviour should be identical.
Does this make sense to have as part of a single commit with D92912 to have a change to populate the LTO configuration options?
I wonder if there is a good way to enumerate the desired set of options (e.g. -O is currently missing), but I don't think that there have been too many attempts to flesh out the argument handling, so we can hold off on that until we find that we have had to do this a few times.
Dec 8 2020
I think that once the LLVM backend target is merged, it would be reasonable to commit this change. The target being experimental is fine, but it should be available. (Marking as requesting changes until the backend is merged).
Dec 7 2020
Maybe I am missing something, but it doesn't seem to be supported in llvm yet:
Dec 3 2020
Seems fine to me. Does the output match what ld64 does?
Dec 2 2020
Following the ACLE is definitely preferable. One thing to verify is if gcc does support this correctly as well. IIRC, it does support __ARM_FP properly and this should be safe, but please do verify that.
Nice! Thanks @thakis, this is a rather useful debugging aid (for users of the linker).
That really is unfortunate. LLVM generally does a pretty good job of sticking to standard C++. The symbols you claim are missing aren't C++ symbols but rather C symbols. I suppose that this doesn't hurt the other targets.
The change itself is fine, though I think that the commit message can be improved. The shuffling of the code should be called out in the summary IMO.
I still don't really like the environment variable approach to the testing.
Dec 1 2020
Nov 30 2020
Gentle post-holiday reminder :)
Nov 25 2020
Nov 23 2020
clang -target x86_64-unknown-windows-itanium -x c -E - -dM <<< '' | grep _MSC_VER
Would be nice to actually mention that in the commit message and adjust the condition to indicate that (!defined(__MINGW32__) && !defined(__MINGW64__)).