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

Fix cmplog implementation #2439

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Fix cmplog implementation #2439

merged 8 commits into from
Jul 25, 2024

Conversation

R9295
Copy link
Collaborator

@R9295 R9295 commented Jul 24, 2024

only set testcase filepath if filepath is none

only set testcase filepath if filepath is none
@R9295 R9295 changed the title fix cmplog implementation Fix cmplog implementation Jul 24, 2024
@R9295
Copy link
Collaborator Author

R9295 commented Jul 24, 2024

Ah probably need to also accomodate cmplog_extended_instrumentation

@R9295
Copy link
Collaborator Author

R9295 commented Jul 24, 2024

Note: the no_std CI failure has nothing to do with this PR

// other => panic!("Invalid CmpLog shape {}", other),
_ => None,
}
}
} else {
unsafe {
let v0_len = self.vals.fn_operands[idx][execution].v0_len & (0x80 - 1);
Copy link
Member

Choose a reason for hiding this comment

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

why you don't look at len anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From my understanding, this was only necessary due to the length of the v1/v0 array being 31 instead of 32 right?

Copy link
Member

Choose a reason for hiding this comment

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

hmm no.
sometimes each fn operands has its length (not necessarily 32). so you should keep the length

Copy link
Member

Choose a reason for hiding this comment

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

Also, random question, could we get around the allocation here?

Copy link
Member

Choose a reason for hiding this comment

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

not possible.
the tokens have to be copied from the shared mem. else it'll be overwritten in the next round

Copy link
Member

Choose a reason for hiding this comment

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

But they could be copied to a preexisting buffer

Copy link
Member

@tokatoka tokatoka Jul 25, 2024

Choose a reason for hiding this comment

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

but we don't know how many of them needs buffer so we can't decide its size.

Copy link
Member

Choose a reason for hiding this comment

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

It's only up to 64 bytes, so we could just always have 64 bytes extra?

Copy link
Member

Choose a reason for hiding this comment

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

Like, just memcpy the existing struct and make a nice accessor around it that returns a slice

Copy link
Member

Choose a reason for hiding this comment

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

See #2442

@domenukk
Copy link
Member

@R9295 can you fix the length so we can merge?

@domenukk domenukk merged commit 76e1b4c into AFLplusplus:main Jul 25, 2024
20 of 30 checks passed
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