- User Since
- Jan 23 2016, 8:31 AM (182 w, 2 d)
Nov 2 2017
Oct 30 2017
Updated the test to match the format after autoupgrade.
Actually I just realized that the last test is referring to auto upgraded attribute numbers, I'll check if I can directly use new attribute syntax and have more predictable numbers (or do regex matching instead)
Oct 26 2017
Do you need this right away, or can we wait for that change to proceed?
Oct 25 2017
Thanks for the review. Test removed and patch updated. I'll merge some time tomorrow.
Oct 24 2017
Who should make the decision of whether the test should be included or if it's needed? Or how much longer do I still need to wait before merging either with or without the test?
Oct 22 2017
Thanks for the review.
Oct 20 2017
Oct 17 2017
Oct 16 2017
Thanks for review.
But the point that SM does not do that makes me thinking that it might be a separate patch.
Oct 14 2017
Thanks for the review.
I'm hoping for input from a code owner to see whether they think introducing a UnitTest is worth it.
Oct 12 2017
Personally I would be happy with the change without a heavyweight test.
Oct 11 2017
I put together a really simple test based on the only other occurance of streamer in the tests (from DWARF tests). It reliably crashes without the patch and doesn't with the patch. This is about as much as what I can possibly come up with. A similar test can be added for AArch64 but won't be testing anything. I have no idea what result I should test.
Oct 9 2017
I've found another failure in instcombine (see last test) so it seems to me that CastInst::isBitOrNoopPointerCastable is the right place for this logic. This also answers my second question since the cast between address spaces is already handled by isBitCastable
Oct 5 2017
we might want at least an assert here.
My guess is that the only way to plausibly test this may be to add a new unittest maybe under unittest/MC?
One thing I'm a bit puzzled about in D30724 is that line 467 (in review) looks like it will move a LastEMSInfo unique_ptr<ElfMappingSymbolInfo> that points to nullptr on first call as the class member on line 686 isn't initialized. This may or may not be important and I may have missed something in the code.
Why do you need to call this here and in reset()?
Sep 27 2017
There was no comment since the only comment I can add would be repeating the condition itself. I do like to add comment about why this is needed or if this is the only way we can fix the issue but that's also what I'm not sure about (quesiton 1 in my review comment). Anyway, I added a comment to repeat the condition for now.
Sep 26 2017
Jul 29 2017
Use AccessTy and added a test to make sure that the case where we load from an non-integral point is still optimized.
Jun 21 2017
I see the non-arch specific property as the good part since currently everything other than PPC claims to support large model with PIC and just generate wrong code. If a fallback that's always implementable and correct is defined, the only arch-specific changes needed will be for efficiency and not for correctness.
If the jump table is writable
Jun 20 2017
Why would a writable jump table help?
Who is we? I'm moderately sure that is not the case in general, since it would create less efficient code.
PPC changes reverted and tests updated. I believe this addresses all the comment so far.
the existing 32-bit offsets are probably fine.
It puts offsets between the BB and a picbase into the jumptable.
I expect something better could be done for PPC, but this is entirely in line with the existing 32-bit code and correctness comes before performance. I pretty strongly object to characterising the patch as "wrong".
Jun 18 2017
Jun 17 2017
I'm seeing a similar failure caused by the same code even with this patch applied on LLVM 4.0.
Jun 15 2017
Jun 10 2017
Oh and this fixes https://bugs.llvm.org/show_bug.cgi?id=33345
Jan 26 2017
Jan 10 2017
I would expect an IR test for this,
Except you don't?
Rebased to only include the code model part....... Hopefully this time someone can comment on the whole PR instead of half at a time ;-p........
...... Looks like the relocation part was implemented in a later pull request https://reviews.llvm.org/D28122 that's already committed.........................
Jan 5 2017
(i.e. can both llvm and gnu libunwind handle DW_EH_PE_sdata8)
Dec 24 2016
Dec 15 2016
Dec 14 2016
Tests added. Also discovered and had to fix two other bugs since the arch detection was wrong for aarch64_be ELF file....
(and add the required tests please)
Dec 13 2016
Use target endian for data relocation. This one actually depends on https://reviews.llvm.org/D27609 now....
Use target endian for data relocation.
Dec 11 2016
Dec 9 2016
Tests added and endianess issue fixed.
Nov 23 2016
Nov 17 2016
Nov 16 2016
Nov 11 2016
Nov 4 2016
I assume the patch is actually merged even though somehow it doesn't trigger the hook that closes this?
Nov 2 2016
Since this started causing problems after r285426, I'm wondering what the reproducer looks like / why we haven't seen this issue already.
Nov 1 2016
Oct 29 2016
And I think it didn't cause any problem because Error and unique_ptr has the same layout except for the lowest bit. Before https://reviews.llvm.org/rL285426, the lowest bit should never be set when the Expected is checked/assertion is disabled but this is not true anymore for an unchecked Expected on a release build after the change.
Oct 15 2016
Ping. Does this still need more review or is this OK to merge? I'll also need someone else to push for me since I don't have commit access.
Oct 9 2016
Oct 1 2016
Fix bit31 mask....
Sep 30 2016
Your solution is the same way that R_ARM_PREL31 is handled in lld, so this looks fine from a correctness point of view.