Page MenuHomePhabricator

[clang-format] Add BreakBeforeInlineASMColon configuration
Needs RevisionPublic

Authored by anastasiia_lukianenko on Nov 23 2020, 2:04 AM.

Details

Summary

If `true`, colons in ASM parameters will be placed after line breaks.
true:
asm volatile("loooooooooooooooooooooooooooooooooooooooooooooong",

:
: val);

false:
asm volatile("loooooooooooooooooooooooooooooooooooooooooooooong",

: : val);

Diff Detail

Event Timeline

anastasiia_lukianenko requested review of this revision.Nov 23 2020, 2:04 AM
anastasiia_lukianenko created this revision.
Eugene.Zelenko edited reviewers, added: MyDeveloperDay; removed: Restricted Project.Nov 23 2020, 6:56 AM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 6:56 AM
  1. this needs a full context diff (diff -U 999999)
  2. this needs unit tests
MyDeveloperDay requested changes to this revision.Nov 23 2020, 6:58 AM
This revision now requires changes to proceed.Nov 23 2020, 6:58 AM
  1. Diff was made by git diff with full context
  2. Added description in documentation
  3. Added unit test

Do you think we should show what it looks like with more complex asm examples in the tests?

what would the following look like?

asm volatile("rbit %[aVal], %[aVal]" : [aVal] "+r"(val) : "[aVal]"(val));
__asm__ __volatile__("str x0, %0" : "=m"(regs->regs[0]));
asm volatile ("get\t%0,rfsl" stringify(id) : "=d" (val))

asm ("mov %1, %0\n\t"
    "add $1, %0"
    : "=r" (dst) 
    : "r" (src));

For the "purpose of the tape" is there a reason or group of people who prefer this style? (gcc, linux kernel)

Nothing will happen with your examples, because the line length is less than column limit. But if I make parameters longer results will be:

BreakBeforeInlineASMColon = false;

asm volatile("rbffffit %[aVal], %[aVal]" : [aVal] "+r"(val)
             : "[aVaffffl]"(val));
__asm__ __volatile__("str fffffx0, %0"
                     : "=fffffffffffffffffffffm"(regs->regs[0]));
asm volatile("get\t%0,rffffffffsl" stringify(id)
             : "=dfffffffffffffffffffffff"(val))

BreakBeforeInlineASMColon = true;

asm volatile("rbffffit %[aVal], %[aVal]"
             : [aVal] "+r"(val)
             : "[aVaffffl]"(val));
__asm__ __volatile__("str fffffx0, %0"
                     : "=fffffffffffffffffffffm"(regs->regs[0]));
asm volatile("get\t%0,rffffffffsl" stringify(id)
             : "=dfffffffffffffffffffffff"(val))

And asking the question who will use this configuration:
We are trying to use the clang-format approach as a base for Xen [1] style formatting. During the state of testing clang-format with different configurations, we found that some points regarding the Xen coding style are not
configurable. Therefore, we decided to add them (this and other my patches [2], [3]) to be able to make a choice in different cases.

[1] - https://xenproject.org/
[2] - https://reviews.llvm.org/D92168
[3] - https://reviews.llvm.org/D91949

  1. You need to default initialize BreakBeforeInlineASMColon in the LLVM style.
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
       ...
       LLVMStyle.BreakBeforeInlineASMColon  = false;
       ...
  1. You need a "CHECK_PARSE_BOOL" unit test
CHECK_PARSE_BOOL(BreakBeforeInlineASMColon  );
MyDeveloperDay requested changes to this revision.Nov 27 2020, 3:59 AM
This revision now requires changes to proceed.Nov 27 2020, 3:59 AM
MyDeveloperDay added a comment.EditedNov 27 2020, 4:08 AM

My understanding is that the following current behavour

asm("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm volatile(
    "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm("AAAAAAAAAAAAAAAAAAA" : "DEF" : "GHI");

asm volatile("AAAAAAAAAAAAA" : "DEF" : "GHI");

asm volatile("AAAAAAAAAAAAA" : "DEF"(dst) : "GHI"(src));
asm volatile("AAAAAAAAAAAAA" : "DEF"(dst));

asm volatile("AAAAAAAAAAAAA" : [Foo] "DEF"(dst) : [Foo] "GHI"(src));
asm volatile("AAAAAAAAAAAAA" : % [Foo] "DEF"(dst) : % [Foo] "GHI"(src));

should generate as the following (with the option set to true) correct?

asm("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm volatile(
    "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm("AAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm volatile("AAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm volatile("AAAAAAAAAAAAA"
    : "DEF"(dst)
    : "GHI"(src));

asm volatile("AAAAAAAAAAAAA"
    : "DEF"(dst));

asm volatile("AAAAAAAAAAAAA"
    : [Foo] "DEF"(dst) : [Foo] "GHI"(src));

asm volatile("AAAAAAAAAAAAA"
    : %[Foo] "DEF"(dst)
    : %[Foo] "GHI"(src));

is that correct? I highly recommend adding a few more examples in the test to show this behavior isn't just for "loooong" strings

NOTE: there is a bug in the way '%[Foo]' would get formatted to be '% [Foo]'

For now without my patch current behavior is the following:
Your examples listed below:

asm("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm volatile(
    "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm("AAAAAAAAAAAAAAAAAAA" : "DEF" : "GHI");

asm volatile("AAAAAAAAAAAAA" : "DEF" : "GHI");

asm volatile("AAAAAAAAAAAAA" : "DEF"(dst) : "GHI"(src));
asm volatile("AAAAAAAAAAAAA" : "DEF"(dst));

asm volatile("AAAAAAAAAAAAA" : [Foo] "DEF"(dst) : [Foo] "GHI"(src));
asm volatile("AAAAAAAAAAAAA" : % [Foo] "DEF"(dst) : % [Foo] "GHI"(src));

Formatted with clang-format in this way:

asm("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm volatile(
    "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    : "DEF"
    : "GHI");

asm("AAAAAAAAAAAAAAAAAAA" : "DEF" : "GHI");

asm volatile("AAAAAAAAAAAAA" : "DEF" : "GHI");

asm volatile("AAAAAAAAAAAAA" : "DEF"(dst) : "GHI"(src));
asm volatile("AAAAAAAAAAAAA" : "DEF"(dst));

asm volatile("AAAAAAAAAAAAA" : [Foo] "DEF"(dst) : [Foo] "GHI"(src));
asm volatile("AAAAAAAAAAAAA" : % [Foo] "DEF"(dst) : % [Foo] "GHI"(src));

So that's why my patch is breaking only long strings. If this is a bug, I can try to fix it. Then I update my patch so the configuration will be as @MyDeveloperDay expected.

MyDeveloperDay added a comment.EditedDec 5 2020, 2:21 AM

Maybe I'm confused, if you set this setting isn't it to break before ever ASM Colon is that not what is required?

MyDeveloperDay resigned from this revision.Dec 5 2020, 2:23 AM
This revision now requires review to proceed.Dec 5 2020, 2:23 AM
anastasiia_lukianenko added a comment.EditedDec 7 2020, 1:17 AM

BreakBeforeInlineASMColon configuration was created to have an option of removing this break, because without my patch it breaks before ASM colon in long lines. That's why I saved the behavior of BreakBeforeInlineASMColon=true as it was initially.
Therefore, I'm asking you is this a bug - breaking only long lines or this is normal clang-format behavior?

I think my expectation would be that if you are going to have an option BreakBeforeInlineASMColon then it should do it for all case long and short, otherwise its not going to be clear.

MyDeveloperDay requested changes to this revision.Jan 17 2021, 3:18 AM
This revision now requires changes to proceed.Jan 17 2021, 3:18 AM