Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unify usage of inline assembly keywords #5059

Merged
merged 1 commit into from
Apr 1, 2016

Conversation

kaspar030
Copy link
Contributor

From "https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html":

"The asm keyword is a GNU extension. When writing code that can be compiled with -ansi and the various -std options, use __asm__ instead of asm (see Alternate Keywords). "

Stumbled upon this when trying to compile with -std=c11, which doesn't allow asm().

@kaspar030 kaspar030 added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Mar 14, 2016
@kaspar030 kaspar030 added this to the Release 2016.04 milestone Mar 14, 2016
@Kijewski
Copy link
Contributor

In this case I'd rather #define asm __asm__.

@kaspar030
Copy link
Contributor Author

Why, if gcc suggests usage of __asm__?

@Kijewski
Copy link
Contributor

Because it's ugly. Other than that I have no particular reason to prefer a #define over a %s/…/…/g.

@kaspar030
Copy link
Contributor Author

I don't find it ugly, but I also don't care. What would be the prefered location of such a define?

@OlegHahm
Copy link
Member

I find it confusing to define a macro with the same name as a GCC extension.

@Kijewski
Copy link
Contributor

What would be the prefered location of such a define?

Actually, I have no idea. Please ignore my remarks. :)

@jnohlgard
Copy link
Member

__asm__ is still a GCC extension.

This is how CMSIS solves the described issue for different compilers: https://github.com/RIOT-OS/RIOT/blob/master/cpu/cortexm_common/include/core_cm0.h#L79

@haukepetersen
Copy link
Contributor

I would tend to go with __asm__, before having an asm define somewhere.

asm is still a GCC extension

but if it solves the issue for c11, I think we should prefer it over asm()?! I don't have a strong opinion here, but if we want to switch to __asm__, I would definitely prefer to use it directly over some mapping macro.

@jnohlgard
Copy link
Member

👍 for unifying, I don't have a strong opinion on what the outcome is as long as it is unified across the tree.
I guess a well-written regex could fix it for all occurrences.

@kaspar030
Copy link
Contributor Author

This PR should unify as is. Only file I didn't touch is cpu/x86/nop_nop_nop.inc, which I'm not sure is needed at all.

@jnohlgard
Copy link
Member

Btw, from quickly looking at the diff I believe that there may be some missing volatile keywords at some of the inline asm.

@@ -67,7 +67,7 @@ void msp430_init_dco(void)
BCSCTL1 |= DIVA1 + DIVA0; /* ACLK = LFXT1CLK/8 */

for (i = 0xFFFF; i > 0; i--) { /* Delay for XTAL to settle */
asm("nop");
__asm__("nop");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gebart you mean like here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, like there
On Mar 31, 2016 10:33 PM, "Kaspar Schleiser" notifications@github.com
wrote:

In boards/telosb/board.c
#5059 (comment):

@@ -67,7 +67,7 @@ void msp430_init_dco(void)
BCSCTL1 |= DIVA1 + DIVA0; /* ACLK = LFXT1CLK/8 */

 for (i = 0xFFFF; i > 0; i--) {        /* Delay for XTAL to settle */
  •    asm("nop");
    
  •    **asm**("nop");
    

@gebart https://github.com/gebart you mean like here?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/RIOT-OS/RIOT/pull/5059/files/c3f7186d4e686561b3e5714d91819d1b361542cd#r58122804

@kaspar030
Copy link
Contributor Author

Will open an issue. edit #5218

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 31, 2016
@kaspar030
Copy link
Contributor Author

(launching CI to check for build errors)

@jnohlgard
Copy link
Member

ACK

@kaspar030 kaspar030 merged commit 39984de into RIOT-OS:master Apr 1, 2016
@kaspar030 kaspar030 deleted the unify_asm_keyword branch April 1, 2016 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants