Skip to content
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

Expand declaration attributes #68

Merged
merged 17 commits into from
Feb 23, 2019
Merged

Expand declaration attributes #68

merged 17 commits into from
Feb 23, 2019

Conversation

pchickey
Copy link
Collaborator

@pchickey pchickey commented Feb 21, 2019

Based on #67

Adds attributes to each declaration that allow the user to control:

  • Scope is one of Scope { Global, Local, Weak }. Local by default, is_global() still gives a bool. Fixes Weak linkage support #60
  • Visibility is one of Visibility { Default, Protected, Hidden }. Default by default. Fixes External linkage + hidden visibility support #61
  • Optional alignment hint. The alignment hint is used if provided, otherwise the heuristic is used based on whether the section is executable or writable. Fixes custom section alignments #34
  • Data type (strings vs binary data) of Data and DebugSection. Default is binary data.

The data type attribute means:

  • CStrings are now just an attribute on Data, rather than a totally separate variant of decl. The builder API stays the same
  • Debug sections can be marked as containing strings or not, so we can eventaully remove the heuristic that marks .debug_str and .debug_line_str sections as strings automatically. This heuristic is left in place for now to minimize breakage downstream.

Additionally, non-writable data are now put in the .rodata segment, which means the loader will put that data in memory that is protected against writing.

Pat Hickey added 7 commits February 21, 2019 14:46
and, put section_index into SymbolBuilder because many users set it
right after building the symbol anyway
TODO: come up with a plan to deprecate automatically making .debug_str
and .debug_line_str into string sections, because theres a proper api
for that now
@pchickey
Copy link
Collaborator Author

pchickey commented Feb 22, 2019

Integrated with goblin changes and added a test suite.

Found that failure::Error was being used in Artifact::declare instead of ArtifactError, so I switched it to that, which i think is the intention. It will coerce into failure::Error at any use sites that expect that type and use ?. Also, exported ArtifactError.

Once a reviewer approves this PR, I'll bump the faerie version to 0.8.0 and merge it.

Copy link
Collaborator

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Very nice!

src/artifact/decl.rs Outdated Show resolved Hide resolved
src/artifact/decl.rs Outdated Show resolved Hide resolved
src/artifact/decl.rs Outdated Show resolved Hide resolved
src/artifact/decl.rs Outdated Show resolved Hide resolved
@pchickey pchickey merged commit cbdf764 into master Feb 23, 2019
@pchickey pchickey deleted the pch/decl_attributes branch February 23, 2019 19:31
@pchickey
Copy link
Collaborator Author

Thanks for the code review. I published on crates.io as 0.8.0.

@m4b
Copy link
Owner

m4b commented Feb 24, 2019

Wow this was epic great work ! I especially like the testing scaffold you added, thanks for that and all the hard work that went into this !

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.

3 participants