Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[VarDumper] AddFFI\CData andFFI\CType types#46773

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:6.2fromSerafimArts:feature/var-dumper-ffi
Jul 20, 2022
Merged

[VarDumper] AddFFI\CData andFFI\CType types#46773

fabpot merged 1 commit intosymfony:6.2fromSerafimArts:feature/var-dumper-ffi
Jul 20, 2022

Conversation

SerafimArts
Copy link
Contributor

@SerafimArtsSerafimArts commentedJun 25, 2022
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

Added support of FFI:

$ffi = \FFI::cdef(<<<'CPP'    typedef struct { int x; int y; } Point;    typedef struct Example {        uint8_t array[32];        long longVal;        __extension__ union {            __extension__ struct { short shortVal; };            struct { Point point; float e; };        };        bool boolValue;        int (*func)(struct __sub *h);    } Example;CPP);$struct =$ffi->new('Example');$struct->func = (staticfn (object$ptr) =>42);dump($struct);

Before

FFI\CData {#2}

After

FFI\CData<struct Example> size 64 align 8 {#2  +array: FFI\CData<uint8_t[32]> size 32 align 1 {#18}  +int32_t longVal: 0  +int16_t shortVal: 0  +point: FFI\CData<struct <anonymous>> size 8 align 4 {#17    +int32_t x: 0    +int32_t y: 0  }  +float e: 0.0  +bool boolValue: false  +func: [cdecl] callable(struct __sub*): int32_t {#20    returnType: FFI\CType<int32_t> size 4 align 4 {#25}  }}

P.S. I apologize for the multiple force pushes, errors in tests and codestyle have been fixed.

Review And TODOs

  • Pointers
    • Pointer to scalar tests.
    • Pointer to struct tests.
    • "Special" pointer tochar* tests (with\0 and without\0).
  • Possible Errors
    • Do not dump union fields with pointer references (possible SIGSEGV).

Nek-, ging-dev, BoShurik, and lyrixx reacted with heart emojiNek- and ging-dev reacted with rocket emoji
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@lyrixx
Copy link
Member

I wanted to do that for a while. But you add a really better caster than what I have ever dreamed. This is very nice. Thanks!

SerafimArts and damienalexandre reacted with heart emoji

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Really nice, thanks!
Here is some nitpicking.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I spent some time on your patch and submitted a few changes to your fork. Please fetch +reset --hard before opening it again 😬

$ptr = $type->getPointerType();

if (null === $data) {
return [Caster::PREFIX_VIRTUAL.'0' => $ptr];

Choose a reason for hiding this comment

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

I'm not sure I understand what this is about, can you please add a test case to cover it?

Copy link
ContributorAuthor

@SerafimArtsSerafimArtsJun 27, 2022
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understand what this is about

Any pointer contains inside a link to where it points and at least one element:

// Cint *getArray() {    int *array =new int[42];    array[0] =1;    array[1] =2;return array;}// >> \FFI::cdef()extern int *getArray(void);// >> PHP$data =$ffi->getArray();dump($data);// >> VarDumper representationFFI\CData<int32_t*> {#00:FFI\CData<int32_t> {#1        +cdata:1    }}

Although the size and type of the memory area where the pointer refers is not known (I propose to provide an alternative forvoid*, a pointer to something undefined) in advance, there is at least a zero element there (if the memory area is available).

In this case, if theCData was not transferred, a structure based on the type will be formed:

FFI\CType<int32_t*> {#00:FFI\CType<int32_t> {#1 }}

We can also get where the pointer "points" to via array access:

echo$data[0]->cdata;// 1// OR (magic of FFI: alternative, but not strict option)echo$data->cdata;// 1

Ok, ill add a tests

return match ($ptr->getKind()) {
CType::TYPE_CHAR => [Caster::PREFIX_VIRTUAL.'cdata' => self::castFFIStringValue($data)],
CType::TYPE_FUNC => self::castFFIFunction($stub, $ptr),
default => [Caster::PREFIX_VIRTUAL.'cdata' => $data[0]],

Choose a reason for hiding this comment

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

That might be a dumb question as I don't know FFI much, by why$data[0]? Is this tested?

Copy link
ContributorAuthor

@SerafimArtsSerafimArtsJun 27, 2022
edited
Loading

Choose a reason for hiding this comment

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

As I noted above, this is getting real data by a pointer to this data:$data->getType(); // int* and$data[0]->getType(); // int

No, there are no tests for pointers. Can be added, no problem.


$string = implode('', $result);
$stub = new CutStub($string);
$stub->cut = -1;

Choose a reason for hiding this comment

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

I replace the custom truncation by aCutStub.
Can't we know the size of the truncated data btw?
I think this is also not tested, isn't it? Can it be?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Can't we know the size of the truncated data btw?

In most cases, strings contain a trailing\0 (or\0\0 inwchar_t). This is how, for example, the functionFFI::string($pointerToChar); // "string\0" works.

However, this is not entirely safe, since the function can return strings without the terminating\0, and store the length separately:

struct CustomString {    char* data;    int length;}

In this case, while we are reading the data fromchar array, we can get into the "junk memory" or get an Access Violation or Segmentation Fault errors (https://en.wikipedia.org/wiki/Segmentation_fault). Therefore, there is a limit on the length of the line, so that even when reading extra data, we can stop in time.


No, this has only been tested in manual mode. You are right, we need to add to this too.


return match ($ptr->getKind()) {
CType::TYPE_CHAR => [Caster::PREFIX_VIRTUAL.'cdata' => self::castFFIStringValue($data)],
CType::TYPE_FUNC => self::castFFIFunction($stub, $ptr),

Choose a reason for hiding this comment

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

From what I understand, a pointer pointing to a function is going to be dumped the same as dumping a function.
Did I get this right? Shouldn't we make a difference between both?
Do we have the same concern for other kind of pointed values?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

From what I understand, a pointer pointing to a function is going to be dumped the same as dumping a function.

Yep. However, a function is always a pointer to it (seemingly o___0).

It looks something like:

$pointer =function () {... };

The variable contains a reference to a function (i.e. a pointer to it), but just like that, a function does not exist without a pointer to it.

Maybe I'm wrong on this. Need a C/C++ specialist, which I am not :D

Do we have the same concern for other kind of pointed values?

Perhaps arrays with a predetermined size 🤔

@SerafimArts
Copy link
ContributorAuthor

Done.

Perhaps it makes sense to remove the alignment from the output? I have not yet come across cases where this information would be needed.

@OskarStarkOskarStark changed the title[VarDumper] Add FFI\CData and FFI\CType types[VarDumper] AddFFI\CData andFFI\CType typesJul 14, 2022
@SerafimArts
Copy link
ContributorAuthor

@lyrixx ping? =)

Copy link
Member

@lyrixxlyrixx left a comment

Choose a reason for hiding this comment

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

👍🏼 Super cool. Thanks

SerafimArts reacted with hooray emoji
@fabpot
Copy link
Member

Thank you@SerafimArts.

SerafimArts reacted with thumbs up emoji

@fabpotfabpot merged commit485843e intosymfony:6.2Jul 20, 2022
@fabpotfabpot mentioned this pull requestOct 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@lyrixxlyrixxlyrixx approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

5 participants
@SerafimArts@carsonbot@lyrixx@fabpot@nicolas-grekas

[8]ページ先頭

©2009-2025 Movatter.jp