[SAI] wrong code generated in libsai #415

kcudnik · 2023-08-02T10:38:56Z

I found some issues with generated code for libsai.so from templates after enabling compiler warnings:

saidashvip.cpp
saidashacl.cpp: In function 'sai_status_t sai_create_dash_acl_group(sai_object_id_t*, sai_object_id_t, uint32_t, const sai_attribute_t*)':
saidashacl.cpp:39:15: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
   39 |     u16SetVal(objId, mf_exact, 16);
      |               ^~~~~
saidasheni.cpp: In function 'sai_status_t sai_create_eni(sai_object_id_t*, sai_object_id_t, uint32_t, const sai_attribute_t*)':
saidasheni.cpp:200:15: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
  200 |     u16SetVal(objId, mf_exact, 16);
      |               ^~~~~
saidashinboundrouting.cpp: In function 'sai_status_t sai_create_inbound_routing_entry(const sai_inbound_routing_entry_t*, uint32_t, const sai_attribute_t*)':
saidashinboundrouting.cpp:27:31: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
   27 |         u16SetVal(tableEntry->eni_id, mf_exact, 16);
      |                   ~~~~~~~~~~~~^~~~~~
saidashinboundrouting.cpp: In function 'sai_status_t sai_remove_inbound_routing_entry(const sai_inbound_routing_entry_t*)':
saidashinboundrouting.cpp:131:31: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
  131 |         u16SetVal(tableEntry->eni_id, mf_exact, 16);
      |                   ~~~~~~~~~~~~^~~~~~
saidashmeter.cpp: In function 'sai_status_t sai_create_meter_policy(sai_object_id_t*, sai_object_id_t, uint32_t, const sai_attribute_t*)':
saidashmeter.cpp:217:15: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
  217 |     u16SetVal(objId, mf_exact, 16);

...
saidashoutboundrouting.cpp:158:22: note: shadowed declaration is here
  158 |                 auto param = action->add_params();
      |                      ^~~~~
saidashoutboundrouting.cpp:178:26: warning: declaration of 'param' shadows a previous local [-Wshadow]
  178 |                     auto param = action->add_params();
      |                          ^~~~~
saidashoutboundrouting.cpp:172:22: note: shadowed declaration is here
  172 |                 auto param = action->add_params();
      |                      ^~~~~
saidashoutboundrouting.cpp:192:26: warning: declaration of 'param' shadows a previous local [-Wshadow]
  192 |                     auto param = action->add_params();
      |                          ^~~~~
saidashoutboundrouting.cpp:186:22: note: shadowed declaration is here
  186 |                 auto param = action->add_params();
      |                      ^~~~~
saidashoutboundrouting.cpp: In function 'sai_status_t sai_remove_outbound_routing_entry(const sai_outbound_routing_entry_t*)':
saidashoutboundrouting.cpp:275:31: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
  275 |         u16SetVal(tableEntry->eni_id, mf_exact, 16);
      |                   ~~~~~~~~~~~~^~~~~~
saidashvnet.cpp: In function 'sai_status_t sai_create_vnet(sai_object_id_t*, sai_object_id_t, uint32_t, const sai_attribute_t*)':
saidashvnet.cpp:39:15: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
   39 |     u16SetVal(objId, mf_exact, 16);
      |               ^~~~~
saidashpavalidation.cpp: In function 'sai_status_t sai_create_pa_validation_entry(const sai_pa_validation_entry_t*, uint32_t, const sai_attribute_t*)':
saidashpavalidation.cpp:27:31: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
   27 |         u16SetVal(tableEntry->vnet_id, mf_exact, 16);
      |                   ~~~~~~~~~~~~^~~~~~~
saidashpavalidation.cpp: In function 'sai_status_t sai_remove_pa_validation_entry(const sai_pa_validation_entry_t*)':
saidashpavalidation.cpp:105:31: warning: conversion from 'sai_object_id_t' {aka 'long unsigned int'} to 'sai_uint16_t' {aka 'short unsigned int'} may change value [-Wconversion]
  105 |         u16SetVal(tableEntry->vnet_id, mf_exact, 16);
      |

there are 2 issues here:

param variable is shadowed
u16SetVal(objId, mf_exact, 16); and u16SetVal(tableEntry->vnet_id passes uint64_t to uint16_t param, not sure what was intended here but this is an error (i don't know how to fix this, probably some new function needs to be added to utils.cpp)

@marian-pritsak @chrispsommers please take a look and advise how to fix or provide solution

The text was updated successfully, but these errors were encountered:

kcudnik · 2023-08-02T10:58:28Z

After this will be fixed this https://github.com/sonic-net/DASH/pull/416/files#diff-0aa5719be28d4f17eb9bfe12e51198fadd9f7c1b0bf0a36600ffb345e7570d05R15 can be uncommented

chrispsommers · 2023-08-02T20:26:02Z

@marian-pritsak might be the best person to look at this, I think it was his code originally.

kcudnik · 2023-08-03T08:33:35Z

i fixed param shadow warning here https://github.com/sonic-net/DASH/pull/416/files#diff-dc64de3830be25bf26df782f6ecffd85e2c43287b8f26d24d16c23244b6ec31bR361

kcudnik · 2023-08-07T07:25:38Z

ping

kcudnik · 2023-08-15T16:11:22Z

@marian-pritsak please advise how to fix those obj issues

kcudnik · 2023-08-22T19:13:41Z

ping

kcudnik · 2023-08-30T15:37:53Z

ping

KrisNey-MSFT · 2023-08-30T16:46:40Z

Idea was possibly to generate a struct; truncate to lower 16 bits. Will this cause an issue with a duplicate object name/id?
bmv2->sailib, takes only the bits, s/be expected
maybe not generate a warning? Intended to truncate.

kcudnik · 2023-09-08T11:20:23Z

@marian-pritsak please help resolve that, or at least give hints how i can do it myself, i really like to enable warnings as errors in this repo

kcudnik · 2023-09-13T07:19:24Z

ping

kcudnik · 2023-09-20T10:06:35Z

ping

chrispsommers · 2023-09-21T17:59:12Z

Per @marian-pritsak needs some more work to find a better way to perform casting. Deferred for now, it's not "broken."

kcudnik · 2023-09-22T06:21:40Z

yea, thanks, not calming it's broken, just generates warnings during compilation, and i want to enable warnings as errors to eliminate this in the future

kcudnik · 2023-09-27T11:19:32Z

@marian-pritsak i has been 2 months on this, please take care of this, this is only 1 line of code which needs your attention, you alreade mentioned on DASH meetings to help with this, you ether add cast for this fo uint16, or add proper uint64 to take entire oid number,i have no clue about bmv2 implementation so im not able to make this judgment, if you dont want to make PR for this, please make comment here to tell which one is correct and i will make this change

KrisNey-MSFT · 2023-10-04T16:11:15Z

check back next week

KrisNey-MSFT · 2023-10-11T16:07:54Z

check back next week

KrisNey-MSFT · 2023-11-07T17:12:12Z

@AmithGspn has offered to take a look, adding as Assigned

KrisNey-MSFT · 2023-11-13T15:50:49Z

From Meyappan:

Hi Chris,

Patch for issue#415. Compiled without warnings. Yet to test.
Please check for any major gaps, will test and feedback next week.

diff --git a/dash-pipeline/SAI/SAI b/dash-pipeline/SAI/SAI
--- a/dash-pipeline/SAI/SAI
+++ b/dash-pipeline/SAI/SAI
@@ -1 +1 @@
-Subproject commit 683d72720b3318f83c54074edfa501ea6812550d
+Subproject commit 683d72720b3318f83c54074edfa501ea6812550d-dirty
diff --git a/dash-pipeline/SAI/templates/saiapi.cpp.j2 b/dash-pipeline/SAI/templates/saiapi.cpp.j2
index 2ca9a35..cb919b3 100644
--- a/dash-pipeline/SAI/templates/saiapi.cpp.j2
+++ b/dash-pipeline/SAI/templates/saiapi.cpp.j2
@@ -61,7 +61,8 @@ static sai_status_t dash_sai_create_{{ table.name }}(
auto mf = matchActionEntry->add_match();
mf->set_field_id({{table['keys'][0].id}});
auto mf_exact = mf->mutable_exact();

{{table['keys'][0].field}}SetVal(objId, mf_exact, {{table['keys'][0].bitwidth}});

//{{table['keys'][0].field}}SetVal(objId, mf_exact, {{table['keys'][0].bitwidth}});
```
 {{table['keys'][0].field}}SetVal(static_cast<uint{{ table['keys'][0].bitwidth }}_t>(objId), mf_exact, {{ table['keys'][0].bitwidth }});
```
{% else %}
// SAI object table with multiple P4 table keys
// Copy P4 table keys from appropriate SAI attributes
@@ -339,7 +340,14 @@ static sai_status_t dash_sai_create_{{ table.name }}(
mf->set_field_id({{key.id}});
{% if key.match_type == 'exact' %}
auto mf_exact = mf->mutable_exact();

   {{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_exact, {{key.bitwidth}});

   //{{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_exact, {{key.bitwidth}});

```
   {% set keyfield = key.field %}
```
```
   {% set bitwidth = key.bitwidth %}
```

   {% if keyfield in ['ipaddr'] or bitwidth in [24,48] %}

     {{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_exact, {{key.bitwidth}});

```
   {% else %}
```

     {{key.field}}SetVal(static_cast<uint{{key.bitwidth}}_t>(tableEntry->{{ key.sai_key_name | lower }}), mf_exact, {{key.bitwidth}});

   {% endif %}
   {% elif key.match_type == 'lpm' %}
   auto mf_lpm = mf->mutable_lpm();
   {{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_lpm, {{key.bitwidth}});

@@ -496,7 +504,14 @@ static sai_status_t dash_sai_remove_{{ table.name }}(
mf->set_field_id({{key.id}});
{% if key.match_type == 'exact' %}
auto mf_exact = mf->mutable_exact();

   {{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_exact, {{key.bitwidth}});

```
   {% set keyfield = key.field %}
```
```
   {% set bitwidth = key.bitwidth %}
```

   {% if keyfield in ['ipaddr'] or bitwidth in [24,48] %}

      {{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_exact, {{key.bitwidth}});

```
   {% else %}
```

     {{key.field}}SetVal(static_cast<uint{{key.bitwidth}}_t>(tableEntry->{{ key.sai_key_name | lower }}), mf_exact, {{key.bitwidth}});

```
   {% endif %}
```

   //{{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_exact, {{key.bitwidth}});
   {% elif key.match_type == 'lpm' %}
   auto mf_lpm = mf->mutable_lpm();
   {{key.field}}SetVal(tableEntry->{{ key.sai_key_name | lower }}, mf_lpm, {{key.bitwidth}});

Note:
Diwali vacation in India, will be back to work on Tuesday.

KrisNey-MSFT · 2023-11-13T15:51:43Z

Thanks for your contribution!
The best way to submit for review is to submit a pull request to DASH and tag folks for review. We don’t work with patch diffs. Yo can mark the PR “Draft” if you want until you’ve tested it. I can assign reviewers to the PR. If you write in the PR description “Fix ” It automatically creates cross-references which will alert anyone associated with the original issue. Please make sure you’ve given your Github ID to Kristina Moore.

You should be able to test in a few minutes using the workflow described in the READMEs:

make clean
make all
make run-switch
make run-saithrift-server
make run-all-tests

Also, when you push your fork to your own repo, it will run the CI pipeline and do all the building and testing automatically! Just make sure you have “enabled actions” in your fork, see image below. You should ensure everything passes before you submit any PR. When you do submit a PR, the CI actions will be run in the DASH repo proper and appear as PR “checks” status.

kcudnik · 2023-11-13T16:34:53Z

Can autor make a PR?

* main: (75 commits) [dash-SAI] Enable warnings as errors (sonic-net#466) [SAI] wrong code generated in libsai sonic-net#415 (sonic-net#463) Fix incorrect IP in SONiC-DASH HLD VNET to VNET example. (sonic-net#459) DASH pipeline packet flow update proposal. (sonic-net#449) [libsai] Add attr name logging when doing get api (sonic-net#451) Build libsai deb packages in github workflow (sonic-net#450) Add Private Link mapping (sonic-net#327) [SAI] Update SAI submodule to the latest origin/master (sonic-net#446) [dash] Add libsai-debs target to create libsai debian packages (sonic-net#444) update p4 compile dependancy to avoid parallel docker runs (sonic-net#443) [dash] Refactor libsai (sonic-net#438) [dash] Update SAI to latest v1.13 (sonic-net#435) [dash-pipeline] Refactor Makefiles (sonic-net#432) Remove ACL tags from BM (sonic-net#425) [submodule] Update SAI submodule to origin/master (sonic-net#431) [sai-api-gen] Write files only when changes are detected (sonic-net#429) Adds SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION api to dash_underlay_routing (sonic-net#422) [SAI] Add missing check for api initialized [SAI] Print oids in hex form [SAI] Change asserts to return error codes and add missing switch api ...

chrispsommers · 2023-12-06T17:09:07Z

Fixed by #463

kcudnik added the bug Something isn't working label Aug 2, 2023

kcudnik assigned marian-pritsak and chrispsommers Aug 2, 2023

KrisNey-MSFT assigned AmithGspn Oct 31, 2023

meyappank added a commit to meyappank/DASH that referenced this issue Nov 15, 2023

[SAI] wrong code generated in libsai sonic-net#415

743bb65

meyappank mentioned this issue Nov 15, 2023

[SAI] wrong code generated in libsai #415 #463

Merged

chrispsommers pushed a commit that referenced this issue Nov 22, 2023

[SAI] wrong code generated in libsai #415 (#463)

a4009ba

chrispsommers closed this as completed Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SAI] wrong code generated in libsai #415

[SAI] wrong code generated in libsai #415

kcudnik commented Aug 2, 2023

kcudnik commented Aug 2, 2023

chrispsommers commented Aug 2, 2023

kcudnik commented Aug 3, 2023 •

edited

Loading

kcudnik commented Aug 7, 2023

kcudnik commented Aug 15, 2023

kcudnik commented Aug 22, 2023

kcudnik commented Aug 30, 2023

KrisNey-MSFT commented Aug 30, 2023 •

edited

Loading

kcudnik commented Sep 8, 2023

kcudnik commented Sep 13, 2023

kcudnik commented Sep 20, 2023

chrispsommers commented Sep 21, 2023

kcudnik commented Sep 22, 2023

kcudnik commented Sep 27, 2023 •

edited

Loading

KrisNey-MSFT commented Oct 4, 2023

KrisNey-MSFT commented Oct 11, 2023

KrisNey-MSFT commented Nov 7, 2023

KrisNey-MSFT commented Nov 13, 2023

KrisNey-MSFT commented Nov 13, 2023

kcudnik commented Nov 13, 2023

chrispsommers commented Dec 6, 2023

[SAI] wrong code generated in libsai #415

[SAI] wrong code generated in libsai #415

Comments

kcudnik commented Aug 2, 2023

kcudnik commented Aug 2, 2023

chrispsommers commented Aug 2, 2023

kcudnik commented Aug 3, 2023 • edited Loading

kcudnik commented Aug 7, 2023

kcudnik commented Aug 15, 2023

kcudnik commented Aug 22, 2023

kcudnik commented Aug 30, 2023

KrisNey-MSFT commented Aug 30, 2023 • edited Loading

kcudnik commented Sep 8, 2023

kcudnik commented Sep 13, 2023

kcudnik commented Sep 20, 2023

chrispsommers commented Sep 21, 2023

kcudnik commented Sep 22, 2023

kcudnik commented Sep 27, 2023 • edited Loading

KrisNey-MSFT commented Oct 4, 2023

KrisNey-MSFT commented Oct 11, 2023

KrisNey-MSFT commented Nov 7, 2023

KrisNey-MSFT commented Nov 13, 2023

KrisNey-MSFT commented Nov 13, 2023

kcudnik commented Nov 13, 2023

chrispsommers commented Dec 6, 2023

kcudnik commented Aug 3, 2023 •

edited

Loading

KrisNey-MSFT commented Aug 30, 2023 •

edited

Loading

kcudnik commented Sep 27, 2023 •

edited

Loading