Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0000411Taste[All Projects] ASN.1 Compilerpublic2015-01-26 09:312018-07-05 08:35
Reporterttsiodras 
Assigned Togmamais 
PrioritynormalSeverityfeatureReproducibilityN/A
StatusfeedbackResolutionreopened 
PlatformOSOS Version
Summary0000411:

Improve rename policy for enumerants

Description

Improve the so-called “rename policy” for enumerants of CHOICE / ENUMERATED types – thus providing consistency between C/Ada.

TagsNo tags attached.
Attached Files

- Relationships
related to 0000773closedgmamais 

Renaming policy - forbidden keywords in fields and same field name/type name

 

-  Notes
(0002387)
gmamais (developer)
2015-07-13 13:26
edited on: 2016-04-15 07:54

I am not sure what is required here. Let me summarize the current situation.
Suppose an ASN.1 grammar with one Enumerated type

RGBCOLORS ::= ENUMERATED {
    red,
    green,
    blue
}

The generated C and Ada types are

typedef enum {
    red = 0,
    green = 1,
    blue = 2
} RGBCOLORS;

SUBTYPE RGBCOLORS_index_range is integer range 0..2;
TYPE RGBCOLORS IS (red, green, blue);
for RGBCOLORS use
    (red => 0, green => 1, blue => 2);
for RGBCOLORS'Size use 32;

The names (as well as the values) of the enumerants are the same.

Suppose an ASN.1 grammar with two Enumerated types with one common enumerant
e.g.

RGBCOLORS ::= ENUMERATED {
    red,
    green,
    blue
}

OTHERCOLORS ::= ENUMERATED {
    red,
    cyan,
    magenta
}

The generated C and Ada types are

typedef enum {
    RGBCOLORS_red = 0,
    green = 1,
    blue = 2
} RGBCOLORS;
typedef enum {
    OTHERCOLORS_red = 0,
    cyan = 1,
    magenta = 2
} OTHERCOLORS;

SUBTYPE RGBCOLORS_index_range is integer range 0..2;
TYPE RGBCOLORS IS (RGBCOLORS_red, green, blue);
for RGBCOLORS use
    (RGBCOLORS_red => 0, green => 1, blue => 2);
for RGBCOLORS'Size use 32;

SUBTYPE OTHERCOLORS_index_range is integer range 0..2;
TYPE OTHERCOLORS IS (OTHERCOLORS_red, cyan, magenta);
for OTHERCOLORS use
    (OTHERCOLORS_red => 0, cyan => 1, magenta => 2);
for OTHERCOLORS'Size use 32;

Again the names match in C and Ada. Please note that the common enumerated item (red) has been renamed to red otherwise there would be a name class.

Choices follow the same principle as enums.

So, what should we change?

(0002388)
maxime (administrator)
2015-07-13 13:34

The "rename policy" was discussed over mail a couple of years ago. Here is the thread:

RE: [taste-dev] r6613 - trunk/DMT/asn1scc.3
George Mamais
to:
'Maxime Perrotin'
10/06/2013 12:04
Cc:
ttsiodras
Hide Details
From: "George Mamais" <gmamais@semantix.gr>
To: "'Maxime Perrotin'" <Maxime.Perrotin@esa.int>
Cc: <ttsiodras@semantix.gr>
History: This message has been replied to.

Yes you are right.

I will add also the no rename policy

From: Maxime Perrotin [mailto:Maxime.Perrotin@esa.int]
Sent: Monday, June 10, 2013 12:53 PM
To: George Mamais
Cc: ttsiodras@semantix.gr
Subject: Re: [taste-dev] r6613 - trunk/DMT/asn1scc.3

I would suggest a third policy, for Ada, without any prefix, since they are not required by the language (policy A or B are only needed because of SPARK, if I understood correctly, but they add unnecessary noise to the code).

No?

On Mon, 10 Jun 2013 11:45:36 +0200, George Mamais <gmamais@semantix.gr> wrote:

Hi Maxime,

This is to let you know, that in the next release of the ASN.1 compiler, I plan to change this behavior.

In particular the user will be able to set rename policy:

Policy A (existing one, i.e. Mychoice_a_PRESENT, b_PRESENT, etc)

Policy B (existing one, i.e. Mychoice_a_PRESENT, Mychoice_b_PRESENT, etc)

The default policy for C will be Policy A (in order to avoid breaking existing code), while the default policy for Ada (where the existing user code is much less) will be Policy B.

Please tell me if you are OK with this.

Kind regards,

George

From: George Mamais [mailto:gmamais@semantix.gr]
Sent: Thursday, June 06, 2013 5:55 PM
To: 'Maxime Perrotin'
Cc: 'ttsiodras@semantix.gr'
Subject: RE: [taste-dev] r6613 - trunk/DMT/asn1scc.3

You are right it would be better to have the behavior you describe.

However, that behavior was adopted in the previous version ASN.1 compiler and the new compiler copied that behavior (otherwise user code would break).

Kind regards,

George

From: Maxime Perrotin [mailto:Maxime.Perrotin@esa.int]
Sent: Thursday, June 06, 2013 4:22 PM
To: George Mamais
Cc: ttsiodras@semantix.gr
Subject: Re: [taste-dev] r6613 - trunk/DMT/asn1scc.3

But then it would be better to have a consistent naming. All enumerant should be built with the same rules to make the code using a particular type portable. In the example below, having Mychoice_a_PRESENT next to b_PRESENT is confusing, and makes it impossible to reuse the same type and code if the type is copied in a different ASN.1 grammar (since the naming is computed based on the neighbouring types).

On Thu, 06 Jun 2013 15:20:03 +0200, George Mamais <gmamais@semantix.gr> wrote:

Hi Maxime,

Indeed, in Ada is not need.

However, Spark has problem with this (it is an error), so I decided to handled it like in C.

Piotr grammar has enums with the same enumerant names, so this how I discovered this issue.

Kind regards,

George

From: Maxime Perrotin [mailto:Maxime.Perrotin@esa.int]
Sent: Thursday, June 06, 2013 3:47 PM
To: George Mamais
Cc: ttsiodras@semantix.gr
Subject: Fwd: [taste-dev] r6613 - trunk/DMT/asn1scc.3

Hi George,

From the log of the new version of the compiler, I noticed this (from Thanassis):

  • emum names are not required to be unique

I do not understand - this creates a regression in some code because the naming of the choice determinant (in Ada) is now prefixed with the type name in some cases, like in C :

TYPE asn1SccMyChoice_selection IS (MyChoice_a_PRESENT, b_PRESENT);

Is there a reason for that ? In Ada, this should not be needed, contrary to C (and until now it was working well without it).

Thanks

Maxime

(0002389)
gmamais (developer)
2015-07-14 08:59

fixed in version 3.2.55

A new command line argument -renamePolicy was added.
-renamePolicy 0 => no rename, (Ada default)
-renamePolicy 1 => rename only conflicting enumerants (C default).
-renamePolicy 2 => rename all enumerants of an enum with at lest one conflicting enumerant.

(0002392)
maxime (administrator)
2015-07-14 14:33

Thanks, closing

(0002404)
maxime (administrator)
2015-08-07 08:03

There is still a rename policy issue:

TASTE-Dataview DEFINITIONS ::=
BEGIN
MyEnum ::= ENUMERATED { hello (0), world }
hello INTEGER ::= 0
END

$ asn1.exe -c -atc -uPER dataview.asn
$ make

-> will complain about the redefinition of "hello"

(0002435)
gmamais (developer)
2015-09-18 08:56

Yes you are right.
How should we handle this case?
(a) emit error
(b) rename enum
(c) rename integer value assignment

I would go for (b) using the enum rename policy provided by user (or the default one if not provided).

(0002437)
maxime (administrator)
2015-09-18 08:58

Agreed to go for (b)

(0002439)
gmamais (developer)
2015-09-18 10:00

fixed in version 3.2.66

(0002445)
maxime (administrator)
2015-09-21 07:16

Thanks! Closing

(0003192)
maxime (administrator)
2018-01-18 14:10

Re-opening.. We miss the rename policy applied to named INTEGERs :

A ::= INTEGER { hello (1) } (0..100)
B ::= INTEGER { hello (2) } (0..100)

this causes an error of the compiler (duplicate definition of hello)

shouldn't the rename policy name the constants TYPE_constant ?
(Here : A_hello and B_hello)

(0003359)
gmamais (developer)
2018-06-12 18:01

This will not be an easy fix because I am not really sure what I have to do. I read the book of Mr. Dubuisson about named INTEGERs. Here the relevant text:
"ASN.1, therefore, provides a specific syntax for the INTEGER type:
the list of integers, where every one of them is preceded by its identifier
(which begins with a lower-case letter); this list is denoted in curly
brackets after the keyword INTEGER.
The error code of a floppy can then be modeled with the type:
ErrorCode ::= INTEGER { disk-full(1), no-disk(-1), disk-not-formatted(2) }

stupid-error ErrorCode ::= disk-full

these identifiers have only a local scope, i.e. they can only be used to
define values of type ErrorCode like stupid-error for example. The same
number can obviously not be named by two different identifiers and a
given identifier cannot name two different numbers. The named integers are not necessarily ordered or consecutive and the list of integers in curly brackets is not restrictive
"

The key word is that these identifiers have only a local scope.

Unfortunately, the current implementation of ASN1SCC does not handle named INTEGERs as local scope values but as global value assignments. I.e. it transforms them into value assignments as follows:
hello INTEGER ::= 1
hello INTEGER ::= 2
(which of course, in this case, does not make any sense and the compiler emits the error)

In our ASN.1 compiler, it is even possible to do the following

B ::= INTEGER { hello (20) } (0..100)
C ::= SEQUENCE (SIZE (0..hello)) OF INTEGER

So, the problem is not the lack of renaming (as we do in enums) but the fact that named INTEGERs are handled like global value assignments. Now, if we implement the ASN.1 standard as described in Mr. Dubuisson's book then the only actual use of named Integers is to define other value assignments such as
stupid-error ErrorCode ::= disk-full
(this is an example of Mr Dubuisson, not mine)
which is very limited. In this case, named INTEGERs values will not appear in the generated code as constants (only the stupid-error will appear) and hence there is no need for any renaming. Which means that if the protocol designer of the PUS had in mind another usage for hello, then this will not appear in the generated code.

What I am trying to say is that perhaps this is not a high priority issue and can be easily handled if we change the grammar.

Please tell me your thoughts on this issue.

(0003360)
maxime (administrator)
2018-06-12 20:21
edited on: 2018-06-12 20:23

A think a local scope means that if you have two different types you cannot assign a value of the first type to a variable of the other type.

so indeed if you declare:

A ::= INTEGER { hello(1) } (0..255)
B ::= INTEGER (0..255)

In the code it should be forbidden to have a variable of type B and assign value hello to it.

Of course I know that C does not respect this basics of data typing...

With Ada, this is enforced, if you have something like

type A is new integer range 0..255;
type B is new integer range 0..255;
A_hello : constant A := 1;
var     : B := A_hello; -- Compilation error

With that in mind, it does make sense to have a systematic renaming (TypeName_ConstantName, as in A_hello)

Without renaming, the original case:

A ::= INTEGER { hello (1) } (0..100)
B ::= INTEGER { hello (2) } (0..100)

which is valid ASN.1, had to chance to exist in C, since it is forbidden to declare two constants of the same name with different values.

A trick in Ada would make it possible using polymorphism:

type A is new integer range 0..100;
type B is new integer range 0..100;
function hello return A is (1);
function hello return B is (2);

c : B := hello;  -- will assign 2
d : A := hello;  -- will assign 1

This is nice (including syntactic sugar - no parenthesis), and the compiler would probably optimize things out to have no overhead (function call) at runtime but since this is not possible in C, let's not go there :-(

Back to our problem:

If you translate into:

hello INTEGER ::= 1
hello INTEGER ::= 2

instead of

hello A ::= 1
hello B ::= 2

then indeed there is no chance that it works as local scope values

Does a translation to

A-hello A ::= 1
B-hello B ::= 2

imply complex changes on your side?
I think that would make it comply to the ASN.1 semantics that you explained from the book of Mr. Dubuisson

Perhaps I am missing something. If you think it's the case, a short term solution is to reject grammars that have these kind of conflicting named integers constants with an error message (rather than at compile time).

(0003361)
gmamais (developer)
2018-06-13 05:55
edited on: 2018-06-13 06:44

But suppose someone defines the following

A ::= INTEGER { hello (1) } (0..100)
B ::= INTEGER { hello (2) } (0..100)
C ::= SEQUENCE (SIZE(1..hello)) OF INTEGER

then what is the size of C 1 or 2?

(0003362)
maxime (administrator)
2018-06-13 06:44

I think this construct is illegal in ASN.1

The right way would be to declare a standalone constant for this one

max-size INTEGER ::= 5
C ::= SEQUENCE (SIZE (1..max-size)) OF INTEGER

And in the generated code, you replace max-size with the actual value (this is what you do already).

(0003363)
gmamais (developer)
2018-06-13 07:14

So, what is the usage of named integers such as hello within the ASN.1?
In my understanding named integers can only be used to define other value assignments such as

other-hello A ::= hello

which is as I said before is very limited.

Anyway, to recap I propose to do the following changes:

1) Named integers will no more be translated to value assignments
2) Named integers will be mapped to integer named items that will have only local scope (same as Enumerated). These named items can be used only in the scope of the specific INTEGER type (i.e. to define value assignments of this type or to define constraints of this type). This is a big change since it requires changes in the definition of the INTEGER AST and then to all places that INTEGER is accessed
3) Named integers will be mapped to the target language as constants (same thing as now)
4) If there are conflicts within named integers, then these will be resolved like enumerated.

Please tell me if you agree with this approach.

(0003364)
maxime (administrator)
2018-06-13 07:32
edited on: 2018-06-13 07:33

I don't really understand point 2. I do not think you need to do anything different from enumerated values.

If you have:

A ::= INTEGER { hello (-1), world (10) } (-10..100)

The only thing you need to do is create two AST entries with value assignments:

a-hello A ::= -1
a-world A ::= 10

And if you had some places in the model with:

myVal A ::= hello

you need to replace hello with value -1, after checking it's either part of A or defined as a constant. And if there is a conflict, i.e. is is both a named value of A and a module-level constant then favour local scope, i.e. take the named value of A, and raise a warning (hello is defined twice, using the type's named value)

Regarding point 4, I think it's better to apply the prefix with the type name systematically, for consistency (i.e. always Typename_constant)

However, if the user has defined a constant named a-hello already, then there is no other option than raising an error.

(0003365)
gmamais (developer)
2018-06-13 08:46
edited on: 2018-06-13 10:44

I have to do what I do for enumerated types.

Let me give an example:

RGB ::= ENUMERATED {red, green, blue}
blueValue RGB ::= blue
MySeq ::= SEQUENCE{
    a RGB (red | blueValue)
}

when the compiler process the constraint (red|blueValue) it has to decide whether each reference is enumerated value defined in the scope of this type (red) or a value assignment (blueValue).

On the other hand, when processing Integer values (in the scope of constraints or value assignments) the compiler checks only for value assignments. This has to change and make INTEGER type to behave more or less as enumerated. This check is made at various places (constraints, DEFAULT values, value assignments, composite values etc).

The current (frontend) AST has to change

Asn1TypeKind =
   | Integer
   | Enumerated        of list

to

Asn1TypeKind =
   | Integer           of list
   | Enumerated        of list

I hope I made my point more clear.
I am not saying that I am not willing to do this modification. I just want you to understand that this change has a bigger impact than just prefixing the names of named INTEGER with the type name.

(0003370)
maxime (administrator)
2018-06-13 14:18

Ok, thanks for the explanations. Keep it low priority if it implies too many changes.


- Issue History
Date Modified Username Field Change
2015-01-26 09:31 ttsiodras New Issue
2015-01-26 09:31 ttsiodras Status

new => assigned

2015-01-26 09:31 ttsiodras Assigned To

=> ttsiodras

2015-04-16 08:26 user28 Assigned To

ttsiodras => user28

2015-06-24 16:12 gmamais Assigned To

user28 => gmamais

2015-07-13 13:26 gmamais Note Added: 0002387
2015-07-13 13:34 maxime Note Added: 0002388
2015-07-14 08:59 gmamais Note Added: 0002389
2015-07-14 08:59 gmamais Status

assigned => resolved

2015-07-14 08:59 gmamais Resolution

open => fixed

2015-07-14 14:33 maxime Note Added: 0002392
2015-07-14 14:33 maxime Status

resolved => closed

2015-08-07 08:03 maxime Note Added: 0002404
2015-08-07 08:03 maxime Status

closed => feedback

2015-08-07 08:03 maxime Resolution

fixed => reopened

2015-09-18 08:56 gmamais Note Added: 0002435
2015-09-18 08:58 maxime Note Added: 0002437
2015-09-18 10:00 gmamais Note Added: 0002439
2015-09-18 10:00 gmamais Status

feedback => resolved

2015-09-18 10:00 gmamais Resolution

reopened => fixed

2015-09-21 07:16 maxime Note Added: 0002445
2015-09-21 07:16 maxime Status

resolved => closed

2016-04-15 07:53 ttsiodras Note Edited: 0002387 View Revisions
2016-04-15 07:54 ttsiodras Note Edited: 0002387 View Revisions
2016-04-15 07:54 ttsiodras Note Edited: 0002387 View Revisions
2018-01-18 14:10 maxime Note Added: 0003192
2018-01-18 14:10 maxime Status

closed => feedback

2018-01-18 14:10 maxime Resolution

fixed => reopened

2018-06-12 18:01 gmamais Note Added: 0003359
2018-06-12 20:21 maxime Note Added: 0003360
2018-06-12 20:22 maxime Note Edited: 0003360 View Revisions
2018-06-12 20:23 maxime Note Edited: 0003360 View Revisions
2018-06-13 05:55 gmamais Note Added: 0003361
2018-06-13 06:44 maxime Note Added: 0003362
2018-06-13 06:44 maxime Note Edited: 0003361 View Revisions
2018-06-13 07:14 gmamais Note Added: 0003363
2018-06-13 07:32 maxime Note Added: 0003364
2018-06-13 07:33 maxime Note Edited: 0003364 View Revisions
2018-06-13 08:46 gmamais Note Added: 0003365
2018-06-13 10:44 maxime Note Edited: 0003365 View Revisions
2018-06-13 14:18 maxime Note Added: 0003370
2018-07-05 08:35 maxime Relationship added

related to 0000773



Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker