Skip to content

Issue when trying to compile asn1 files for s1ap protocol #14

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

Open
yochayKen opened this issue Apr 14, 2024 · 8 comments
Open

Issue when trying to compile asn1 files for s1ap protocol #14

yochayKen opened this issue Apr 14, 2024 · 8 comments

Comments

@yochayKen
Copy link

Hello everyone!

I'm having an issue when I'm trying to compile asn1 files of s1ap protocol through the CLI tool. I'm using the files from the Wireshark repository.

The error message that I'm getting is:
Encountered error while parsing MatchingError(Tag) - Error matching ASN syntax while parsing:SONInformationReply-ExtIEs S1AP-PROTOCOL-EXTENSION

Which appears in the S1AP-IEs.asn file.

Another issue that I get:
Encountered error while parsing MatchingError(Tag) - Error matching ASN syntax while parsing:S1AP-ELEMENTARY-PROCEDURES-CLASS-1 S1AP-ELEMENTARY-PROCEDURE

from S1AP-PDU-Descriptions.asn file.

Thanks for anyone who can help!

@6d7a
Copy link
Member

6d7a commented Apr 16, 2024

Thank you for your issue. I could not reproduce your exact issue with the asn sources from the wireshark repo. However, while our online compiler does produce bindings, I noticed that they still contain some errors that we will need to look into. Unfortunately, the mobile telephony standards are notoriously complicated. I will add the S1AP to our test set, so that we can generate working bindings ASAP.

@v0-e
Copy link
Contributor

v0-e commented Jul 8, 2024

@yochayKen seems to be trying to compile a more recent version of S1AP, where S1AP-ELEMENTARY-PROCEDURES-CLASS-1 is extended. Link

@v0-e
Copy link
Contributor

v0-e commented Jul 10, 2024

Apparently there are also two different types with basically the same name, ECGIList and ECGI-List which get compiled into the same Rust type ECGIList, conflicting with each other.
Maybe it is best to preserve the underscores in to_rust_title_case() @6d7a?

@6d7a
Copy link
Member

6d7a commented Jul 12, 2024

In my opinion, the compiler should catch naming conflicts in the linking and validation phase. If we only preserve underscores, we still get naming conflicts for edge cases like Type-name and Type_name. Also we would sacrifice rust's title case convention for a few edge cases. We do preserve the original names throughout the compilation, so if the compiler can detect and handle a conflict once it has collected all top-level type definitions.

@v0-e
Copy link
Contributor

v0-e commented Jul 12, 2024

Underscores are illegal in ASN.1 identifiers I think. So Type_name as a Rust type could safely represent the Type-name ASN.1 type.
There are no conflicts in S1AP, however the compiler creates one by changing the hyphen to an underscore and then removing it from ECGI-List.

@6d7a
Copy link
Member

6d7a commented Jul 15, 2024

You're right, I remembered the identifier rules wrong. I'm still not sure whether we should throw rust's camel case convention overboard. On the one hand, I like the hyphen-to-underscore conversion because, glancing over a spec, it is visually very close to the original names (and JER also uses it). On the other hand, we are neither following rust's naming convention, nor are we conserving the original names. I've seen that other compilers like asn1c have configuration options for leaving the decision regarding identifier transformations up to the user. Perhaps that's the way to go. What do you think?

@v0-e
Copy link
Contributor

v0-e commented Jul 15, 2024

Since ABCD, AB-CD, ABcd, AB-cd, Ab-cd, Ab-Cd, ..., are all valid and distinct ASN.1 types, then we cannot force the cohersion to title-case/PascalCase everytime.
Yeah, I think it would be cool to have a few options regarding this:

  1. Just convert hyphens to underscores;
  2. Convert hyphens to underscores, iif, the compiler detects any conflicts, otherwise use PascalCase. (default option?)
  3. Force PascalCase, but warn the user if and which conflicts exist so these may be resolved manually in the generated bindings.

@XAMPPRocky
Copy link
Contributor

I've seen that other compilers like asn1c have configuration options for leaving the decision regarding identifier transformations up to the user. Perhaps that's the way to go. What do you think?

FWIW I think that it should follow Rust conventions, and not provide as an option. asn1c provides that option because there is no standard convention in C/C++, there are multiple valid conventions. In Rust there is only one valid convention.

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

No branches or pull requests

5 participants
@XAMPPRocky @6d7a @yochayKen @v0-e and others