Skip to content

Add punycode encoding to v0 mangling #2535

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 18, 2023
Merged

Add punycode encoding to v0 mangling #2535

merged 1 commit into from
Aug 18, 2023

Conversation

tamaroning
Copy link
Contributor

@tamaroning tamaroning commented Aug 7, 2023 •
edited
Loading

Depends on #2533
Please review the last commit

gcc/rust/ChangeLog:

	* backend/rust-mangle.cc (v0_add_identifier): Added punycode encoding
	(v0_mangle_item): Likewise.
	* resolve/rust-ast-resolve-toplevel.h: fix typo
	* rust-session-manager.cc (Session::load_extern_crate): fix typo
	* util/rust-canonical-path.h: fix typo
	* util/rust-hir-map.cc (NodeMapping::get_error): fix typo
	(Mappings::Mappings): fix typo
	* util/rust-mapping-common.h (UNKNOWN_CREATENUM): fix typo
	(UNKNOWN_CRATENUM): Change 0 to UINT32_MAX

I manually checked the following cases for add_v0_identifier:

_あ to u5___x7t
my_crate__ to 10my_crate__
6foobar
foobar to 6foobar
あ to u3l8j
_あいう to u7___x7tgi
ああああ to u6l8jaaa

tamaroning
tamaroning commented Aug 7, 2023
View reviewed changes
gcc/rust/util/rust-mapping-common.h
Comment on lines 63 to 65

#define UNKNOWN_CREATENUM ((uint32_t) (0))
#define UNKNOWN_CRATENUM ((uint32_t) (UINT32_MAX))
#define UNKNOWN_NODEID ((uint32_t) (0))
Copy link
Contributor Author

@tamaroning tamaroning Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I changed this value because CrateNumItr is initialized to 0, which means a crate compiled first is treated as UNKNOWN_CRATENUM.
ref: #894

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philberty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure but the same might be also true for UNKNOWN_NODEID, UNKNOWN_HIRID, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I changed this value because CrateNumItr is initialized to 0, which means a crate compiled first is treated as UNKNOWN_CRATENUM. ref: #894

I'm not a fan of putting special values, they should be treated as a completely separate thing (ehehe, rust habits I guess). For now I think this seems mostly ok. At least I can't think of a situation where it break in a realistic scenario. But in the future we might need to handle this properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CrateNum field was added to CanonicalPath for the v0 mangling because the field is used as disambiguator in demangled symbols.
Actually CanonicalPath::get_crate_num internally calls the following assert and fails here:
https://github.com/Rust-GCC/gccrs/blob/e55113ea2bf0cec2f8436a576ce6f2bcaacd1c27/gcc/rust/util/rust-canonical-path.h#L174C50-L174C50
So I have had to modify this value to avoid this assert failure.

Copy link
Member

Choose a reason for hiding this comment

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

Here thats a good idea @tamaroning raise an issue to do the same with the other ID's

@philberty philberty requested a review from P-E-P August 9, 2023 09:21
P-E-P
P-E-P reviewed Aug 11, 2023
View reviewed changes
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

I was reviewing this when I stumbled on thing I commented in a previous PR, this has'nt been rebased yet, hence my confusion. I'll review this again when conflicts will be fixed and the branch rebased. So far, there's not much to say, that's great work!

gcc/rust/util/rust-mapping-common.h
Comment on lines 63 to 65

#define UNKNOWN_CREATENUM ((uint32_t) (0))
#define UNKNOWN_CRATENUM ((uint32_t) (UINT32_MAX))
#define UNKNOWN_NODEID ((uint32_t) (0))
Copy link
Member

Choose a reason for hiding this comment

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

I changed this value because CrateNumItr is initialized to 0, which means a crate compiled first is treated as UNKNOWN_CRATENUM. ref: #894

I'm not a fan of putting special values, they should be treated as a completely separate thing (ehehe, rust habits I guess). For now I think this seems mostly ok. At least I can't think of a situation where it break in a realistic scenario. But in the future we might need to handle this properly.

@P-E-P P-E-P added this to the GCC 14 Stage 3 milestone Aug 11, 2023
@tamaroning tamaroning force-pushed the v0-punycode branch 4 times, most recently from bbbac30 to 19f07bc Compare August 11, 2023 09:59
@tamaroning
Copy link
Contributor Author

@P-E-P Thank you for your review. Fixed and rebased on master

@tamaroning tamaroning force-pushed the v0-punycode branch 2 times, most recently from 94e5665 to 1266e97 Compare August 15, 2023 15:38
@tamaroning
Copy link
Contributor Author

@P-E-P
I resolved conflicts

@P-E-P
Copy link
Member

P-E-P commented Aug 15, 2023

@P-E-P
I resolved conflicts

I'm away for a few days. I'll let @CohenArthur review this PR. A brand new look on it can't be bad anyway.

@P-E-P P-E-P requested a review from CohenArthur August 15, 2023 19:30
CohenArthur
CohenArthur approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

Looks good! Good work @tamaroning :D

@tamaroning
gccrs: Add punycode encoding to v0 mangling
443619b 
gcc/rust/ChangeLog:

	* backend/rust-mangle.cc (v0_add_identifier): Added punycode encoding
	(v0_mangle_item): Likewise.
	* lex/rust-lex.cc (assert_source_content): Change type
	(test_buffer_input_source): Change type
	(test_file_input_source): Change type
	* resolve/rust-ast-resolve-toplevel.h: fix typo
	* rust-session-manager.cc (Session::load_extern_crate): fix typo
	* util/rust-canonical-path.h: fix typo
	* util/rust-hir-map.cc (NodeMapping::get_error): fix typo
	(Mappings::Mappings): fix typo
	* util/rust-mapping-common.h (UNKNOWN_CREATENUM): fix typo
	(UNKNOWN_CRATENUM): Change 0 to UINT32_MAX

Signed-off-by: Raiki Tamura <[email protected]>
@tamaroning
Copy link
Contributor Author

@CohenArthur Thanks for your review. fixed.

@CohenArthur CohenArthur added this pull request to the merge queue Aug 18, 2023
Merged via the queue into Rust-GCC:master with commit b1dd53f Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philberty philberty philberty left review comments

@P-E-P P-E-P P-E-P left review comments

@CohenArthur CohenArthur CohenArthur approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
libcore 1.49
Status: Done
Milestone
GCC 14 Stage 3
Development

Successfully merging this pull request may close these issues.
4 participants
@tamaroning @P-E-P @philberty @CohenArthur