LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 35913 - Revert r315899 in the 6.0 branch
Summary: Revert r315899 in the 6.0 branch
Status: RESOLVED MOVED
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: X86 (show other bugs)
Version: 6.0
Hardware: PC All
: P release blocker
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks: release-6.0
  Show dependency tree
 
Reported: 2018-01-11 10:45 PST by Dimitry Andric
Modified: 2018-01-16 14:35 PST (History)
6 users (show)

See Also:
Fixed By Commit(s):

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitry Andric 2018-01-11 10:45:31 PST 
As describe in bug 35741, bug 35749, and bug 35831, https://reviews.llvm.org/rL315899 has caused serious regressions in assembly parsing of x86 lock and rep prefixes.

Though bug 35741 has been fixed, there has been no movement nor reviews on the others, so I would like to request this revision to be reverted in the 6.0 branch.  Post 6.0, the regressions can then be fixed.
Comment 1 Andrew V. Tischenko 2018-01-12 07:22:38 PST 
Sorry, but it seems I don't understand something. This patch does not have any relation to bug 35741, bug 35749, and bug 35831. What's the problem?
Comment 2 Dimitry Andric 2018-01-12 07:28:09 PST 
(In reply to Andrew V. Tischenko from comment #1)
> Sorry, but it seems I don't understand something. This patch does not have
> any relation to bug 35741, bug 35749, and bug 35831.

Yes it does, it causes regressions, as described in those bugs:
* comments after lock/rep prefixes were no longer accepted (fixed now)
* .byte directives after lock/rep prefixes are no longer accepted (still open)
* labels after lock/rep prefixes are no longer accepted (still open)

and I think it is very likely there are other regressions in processing lock/rep prefixes.  For example, having a .s file with just:

        lock

assembles fine with GNU as, or with clang before r315899, but after that change it gives:

justlock.s:2:1: error: unknown token in expression

^

There are probably some other edge cases that I haven't thought of, but which are sure to be used in some software out there...
Comment 3 Andrew V. Tischenko 2018-01-12 07:38:37 PST 
Or I got the issue: I fixed one issue but I missed 2 others. What should I do now to resolve the problem? It was realy complex change in prefix support and we don't have proper test base for all possible cases that's why we can expect simmilar issues (and you wrote about this possibility). 

As I understand I should simply it in trunk, right? And how we can discover other possible problems?
Comment 4 Andrew V. Tischenko 2018-01-12 07:41:47 PST 
Misprint: I should simply fix it in trunk, right?
Comment 5 Dimitry Andric 2018-01-12 08:07:19 PST 
(In reply to Andrew V. Tischenko from comment #4)
> Misprint: I should simply fix it in trunk, right?

Yes, please.  If the fixes are in, we can re-title this PR or close it and create a new one for merging the fixes to the release_60 branch.
Comment 6 Hans Wennborg 2018-01-16 07:36:48 PST 
Dimitry: I'm not keen on reverting on the branch only, especially since the problematic commit landed a while back.

I'd prefer to either get the problematic commit reverted on trunk, and merge the revert, or merge fixes.
Comment 7 Andrew V. Tischenko 2018-01-16 08:21:05 PST 
I already fixed the issue and published the fix for review: D42102 [X86] Allow usage of prefixes as a separate instr. It's really simple fix and I hope it will LGTM soon. Let's keep it in trunk and promote to the branch.
Comment 8 Dimitry Andric 2018-01-16 14:35:59 PST 
Closing this in favor of bug 35976.