Simplify test cases as @MaskRay suggested.

I think rejecting .asciz "a" "b" may be fine for us since there is no real world usage.

Then I am not sure we need a new method parseManyWithOptionalComma. It can be specialized.

Thanks all for the comments. From what I read so far, we agree thatt there is a real need to implement this feature in LLVM so add more test cases based on the comments.

https://github.com/ClangBuiltLinux/linux/blob/f01c30de86f1047e9bae1b1b1417b0ce8dcd15b1/arch/arm/probes/kprobes/test-core.h#L114-L116 makes it sounds like .ascii and .asciz work differently in this regard?

In which case that's confusing and likely to lead to bugs if people make use of the preprocessor in the hopes of concatenating strings but end up with NUL bytes being inserted contrary to what there would be in C. Can we not just fix the assembly to use commas rather than this weird syntax that seems to be a special case for .asciz? You can't write .word 2 2, only .word 2, 2, so why do string directives really need special treatment?

In which case I assume .asciz "foo" "bar" is equivalent to .asciz "foobar" rather than .asciz "foo\0bar" (same for .string), just like C string concatenation, and hence why both syntaxes exist?

Update commit message and add a test case.

It appears that I misunderstood the existing code. It actually supports multiple string arguments, but only when commas are used between the arguments. GAS supports commas or spaces as the separator (or when both are used in the same call such as .ascii "aa", "bb" "cc ). Will refactor my patch and update the commit message.

In D91460#2395030, @jrtc27 wrote:

This needs tests, both to show it accepts multiple strings and to show for each of the three directives that it produces the right output (i.e. is equivalent to multiple individual directives).

Moved the definition of SubsectionNumber to avoid padding bytes. Also updated the comments.

In D69411#2253462, @jcai19 wrote:

In D69411#2253193, @MaskRay wrote:

at the very least, if we want to keep it, it should be reordered as my comment says, and don't repeat the name of the variable

Hi @MaskRay, thanks for the comment. By reordering, do you mean moving the definition of SubsectionNumber right after LayoutOrder? I do not understand what difference that would make, would you care to explain a little bit more? Also what by not repeating the name of the variable, I assume you were referring to the comment right above its definition?

In D69411#2253193, @MaskRay wrote:

at the very least, if we want to keep it, it should be reordered as my comment says, and don't repeat the name of the variable

In D69411#2252965, @nickdesaulniers wrote:

It looks like there may have been unresolved comments from @MaskRay . I'm also curious whether @jyknight or @psmith had parting thoughts? (@jcai19 maybe wait a week for their feedback?)

Address @psmith's comments.

Address more comments.

Address @nickdesaulniers' comments.

Combine test cases into one file.

Address some of @MaskRay's comments.

Set the subsection number for fragments in subsecitons.

(1) Fixed a bug while rebasing the patch last time. The failed unit tests dropped to 2 after the fix.
(2) Updated the 2 failed unit tests.

Limit the scope to fragments in the same subsection (fragments not in any subsections are placed in subsection 0). Fragments are inserted into each subsection in order and the difference of their offsets should be fixed once they are inserted. This is experimental as it breaks many tests. Will address the test failure if this idea is proven correct.

Thanks all!

@jyknight while working on a different issue, I happened to take a look at the implementation of subsection (MCSection::getSubsectionInsertionPoint) and from what I understood, fragments in subsections were always placed after fragments not in any subsection., and fragments in the same subsection (or not in any subsection) were always inserted in order and separate from fragments in other subsections. Is it safe to assume what between two fragments either not in any subsections or in the same subsection will be remain unchanged once they are inserted to the fragment list of a section? If so, can we resolve differences of two labels in fragments with such restriction?

In D82826#2189118, @craig.topper wrote:

LGTM

Renamed a variable name and addressed comments. @craig.topper Please let me know if it looks any better. And thanks for enabling multibyte-NOPs in 64-bit mode.

In D82826#2154511, @MaskRay wrote:

I think I have a very old (I emphasized it again in another comment) comment which isn't addressed (https://reviews.llvm.org/D82826#2126406 )
We should align with GNU as on the largest operand .nops supports.

@reames @craig.topper Hi just want to double check if there are any additional issues I should address. Thanks.

Fixed an assertion message.

Address some of @reames's concerns, and rename variables to avoid confusion.

Issuing multiple-byte NOP for 32-bit mode broke many tests and probably worth a separate patch itself. So this patch will keep the behavior on 32-bit mode unchange for now. This built and passed all the tests.

Refactor X86AsmBackend::getMaximumNopSize based on @craig.topper's comment.

In D82826#2126955, @nickdesaulniers wrote:

I'm having trouble posting comments inline; it seems fabricator won't allow me to "save" comments.

Is it too painful to make the member of MCNopsFragment uint64_t? a la MCStreamer::emitFill

(1) This is a temporary implementation as it breaks the following tests:

In D82826#2126190, @craig.topper wrote:

is the 32-bit different the 2 byte case?

Thanks! Checking if the CPU is 64-bit along FeatureNOPL seemed to work. Will verify more. Also I'd like to point out the difference of 32-bit CPUs.

@MaskRay @nickdesaulniers Thanks for the comments. Will refactor the code once we decide which way to go with the implementation.

@craig.topper I tried to add a new Fragment type and use X86AsmBackend::writeNopData to insert NOP instructions. It however changed the outcome of the test cases I used, as X86AsmBackend::writeNopData only generatssingle-byte nop instructions if X86::FeatureNOPL feature bit is not true, while the maxLongNopLength in the current implementation (from X86MCInstLower) allows long nop instructions up to 10 bytes on a 64-bit processor. Which one is preferred? Also, which target triple should I use to test multiple-byte nop instructions? Thanks.

In D82826#2121595, @craig.topper wrote:

Would it be possible to emit a new Fragment type and leverage the existing NOP emission code in X86AsmBackend.cpp. The code here appears to be copied from X86MCInstLower. Which would mean we would now have 3 places that do almost the same thing.

In D80028#2082105, @kristof.beyls wrote:

I ended up pushing a fix for the racy tests in 7e6f891df85

In D80028#2081258, @efriedma wrote:

LGTM

In D77168#2080296, @jfb wrote:

Two minor corrections, looks good otherwise.

Fix typos.

If we have a mechanism bisecting pragmas, this option will not be needed.

In D77168#2073736, @jfb wrote:

Can you add a test for the diagnostic firing after the correct number of initializations? This should include a few types of auto-init, including VLAs.

Cover VLAs.

In D77168#2069783, @MaskRay wrote:

Do we have a mechanism bisecting pragmas? If yes, we can let that tool add #pragma clang attribute push([[clang::uninitialized]], apply_to = variable)

@jfb Does the current implementation look good? Thanks.

@efriedma Does the current version of the patch look like it is good to go? Thanks.

Updates based on comments.

Fix a typo.

Do not create AArch64MCExpr.

Revert to the implementation before the last version.

Add the expression operand as AArch64MCExpr.

Fix a typo.

Update based on comments.

Just want to follow up and see if anything is missing from the latest patch :)