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

Basic support for assert() #6529

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Basic support for assert() #6529

merged 3 commits into from
Feb 13, 2023

Conversation

hbrodin
Copy link
Collaborator

@hbrodin hbrodin commented Jan 3, 2023

Will intercept the __assert_fail call and call polytracker_end which will update the output file and close it.
At least a partial solution to #6528.

@@ -154,7 +154,8 @@ fun:llrintl=functional

# Functions that produce an output that does not depend on the input (shadow is
# zeroed automatically).
fun:__assert_fail=discard
fun:__assert_fail=uninstrumented
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "uninstrumented" result in? Does this mean the function is stubbed and polytracker does not interact with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The pass treats every function in the uninstrumented category in the ABI list file as conforming to the native ABI. Unless the ABI list contains additional categories for those functions, a call to one of those functions will produce a warning message, as the labelling behavior of the function is unknown.

Source: https://clang.llvm.org/docs/DataFlowSanitizer.html#abi-list

Does this answer your question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you! I didn't realize this is/was the same as the regular DFSan ABI list, my bad.

@@ -97,7 +97,7 @@ template <Section... Sections> class OutputFile {
(SectionMeta{.tag = Sections::tag,
.align = Sections::align_of,
.offset = 0,
.size = Sections::allocation_size})...};
.size = 0})...};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does setting the size to 0 for a section mean the section technically won't exist when output?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so? I guess this was done to have correct information in the case when we have a section that's optional, like Events if function tracing is turned off. I think there should be a header entry for the section, but it should tell you there's no trace events in it and it's size is 0? Am I right @hbrodin ?

Copy link
Contributor

@surovic surovic Jan 5, 2023

Choose a reason for hiding this comment

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

Also we should update the comment above this code to reflect this change :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just the initial value. The final value will be set in the destructor of the OutputFile. That behaviour hasn't changed, it is just that we don't assume the size to be max from the beginning, but rather to be empty.

@surovic surovic self-requested a review January 5, 2023 11:54
Copy link
Contributor

@surovic surovic left a comment

Choose a reason for hiding this comment

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

Just updating the code comment.

Copy link
Collaborator

@kaoudis kaoudis left a comment

Choose a reason for hiding this comment

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

looks like the current changelist made @surovic's requested updates, and I have no further questions!

Will intercept the __assert_fail call and call polytracker_end
which will update the output file and close it.
@kaoudis kaoudis force-pushed the issue-6528-partial branch from 2e236f5 to 5bec4ab Compare January 25, 2023 21:59
@kaoudis kaoudis requested a review from surovic January 26, 2023 21:35
@surovic surovic merged commit 2c963b2 into master Feb 13, 2023
@surovic surovic deleted the issue-6528-partial branch February 13, 2023 11:16
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