Skip to content

Conversation

@cfriedt
Copy link
Contributor

@cfriedt cfriedt commented Nov 10, 2021

Atomics will use long rather than int so that they are 64-bit on 64-bit builds and 32-bit on 32-bit builds.

Required by zephyrproject-rtos/zephyr#39531
Originally at zephyrproject-rtos#14

cc @lgirdwood @lyakh @kv2019i

Atomics will use `long` rather than `int` so that they are 64-bit
on 64-bit builds and 32-bit on 32-bit builds.

Required by zephyrproject-rtos/zephyr#39531

Signed-off-by: Christopher Friedt <[email protected]>
@sofci
Copy link
Collaborator

sofci commented Nov 10, 2021

Can one of the admins verify this patch?

reply test this please to run this test once

@plbossart
Copy link
Member

Can one of the admins verify this patch?

reply test this please to run this test once

test this please

@gkbldcig
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

shall we wait until the Zephyr PR is merged before merging this?

tr_info(&ll_tr, "num_tasks %d total_num_tasks %d",
tr_info(&ll_tr, "num_tasks %ld total_num_tasks %ld",
atomic_read(&sch->num_tasks),
atomic_read(&domain->total_num_tasks));
Copy link
Collaborator

Choose a reason for hiding this comment

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

strictly speaking, this file isn't used with Zephyr so these changes aren't fixing Zephyr builds directly, but it's true, that we need consistency here. OTOH using %ld with 32-bit should actually generate warnings like

/tmp/p.c: In function ‘main’:
/tmp/p.c:8:19: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘int32_t’ {aka ‘int’} [-Wformat=]
    8 |  return printf("%ld\n", x);
      |                 ~~^     ~
      |                   |     |
      |                   |     int32_t {aka int}
      |                   long int
      |                 %d

in XTOS builds but since we don't use __attribute__((__format__(printf,... ,...))) with XTOS builds (so far) we shouldn't be getting these. So, so far this should be safe...

@cfriedt
Copy link
Contributor Author

cfriedt commented Nov 10, 2021

shall we wait until the Zephyr PR is merged before merging this?

The Zephyr fork is a downstream of this project, so the change needs to be merged here first, and then the Zephyr fork of sof needs to be updated, and then the west.yml ref needs to be updated in zephyrproject-rtos/zephyr#39531.

I realize it's a bit confusing, but the idea is to preserve the upstream / downstream relationship and have coherent git history.

Hopefully @nashif can clarify if above does not make sense.

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

Rerun CI, looks like it got stuck.

@cfriedt
Copy link
Contributor Author

cfriedt commented Nov 11, 2021

The zephyr build is expected to fail here.

@lgirdwood
Copy link
Member

@kkarask @wszypelt looks like CI got stuck. Can you restart. Thanks !

@wszypelt
Copy link

wszypelt commented Nov 12, 2021

@lgirdwood done :)

@lgirdwood lgirdwood merged commit 970d7d6 into thesofproject:main Nov 12, 2021
@marc-hb
Copy link
Collaborator

marc-hb commented Nov 18, 2021

Follow-up in #5004 zephyr/atomic.h: add a no-op cast: (long)atomic_read(), please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants