You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2021/05/05 13:13:05 UTC
[GitHub] [plc4x] thomas169 opened a new pull request #244: plc4c: memory plumbing
thomas169 opened a new pull request #244:
URL: https://github.com/apache/plc4x/pull/244
started resolving memory issues pulled up by valgrind.
Currently all IO leaks so doing IO in while loop crashes after not very long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] thomas169 commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
thomas169 commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846930078
It's finished.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846957194
Ok ... so I just pulled your latest changes and updated the branch with the latest changes from develop ... it seems you commented out some code in the code generation for optional fields. It seems this is now causing segmentation faults in the testsuite ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846563921
What's the status on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846968624
So I just pushed the merged branches to "feature/plc4x-memory-cleanup" in the official plc4x repo. As you are now a full committer, I think we should continue there (makes the review, updating, merging simpler)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846971985
But having a look at the code ... I think if we move the malloc to the "simple" path, this should not double-allocate ... correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] thomas169 commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
thomas169 commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-835418261
IO now doesn't leak... so says valgrind.
Lots of changes in PR (I know I can commit now direclty) but would quite like to keep memory alloc changes in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] thomas169 commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
thomas169 commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-832676994
I have not verified changes to template result in what is needed (commend mallocs for optional items), but quite sure it will be fine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] thomas169 commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
thomas169 commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-833558447
can now connect and disconnect wilthout leaks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846975214
Sorry for the delay by the way ... I was consumed with a lot of "work stuff" (TM) ... but really looking forward to integrating your changes. As soon as that's done, I really would love moving the PLC4C directory out of the sandbox and into the root of the project (Making it something we consider somewhat mature)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846969455
Well I think only the parser can know if it needs to alloc memory for the optional field. But code like this will always produce segfaults:
` //dataUnitReferenceNumber = malloc(sizeof(uint8_t));
//if(dataUnitReferenceNumber == NULL) {
// return NO_MEMORY;
//}
*dataUnitReferenceNumber = 0;
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] thomas169 commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
thomas169 commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846972305
Yes that is unintended collateral damage as it not in `s7_message.c` or` copt_paramtmer.c`.
Ill have a look at what the code-gen outputs with changes to templates (I did not before, hence all the explosions) sometime this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz merged pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz merged pull request #244:
URL: https://github.com/apache/plc4x/pull/244
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz edited a comment on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz edited a comment on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846969455
Well I think only the parser can know if it needs to alloc memory for the optional field. But code like this will always produce segfaults:
`
//dataUnitReferenceNumber = malloc(sizeof(uint8_t));
//if(dataUnitReferenceNumber == NULL) {
// return NO_MEMORY;
//}
*dataUnitReferenceNumber = 0;
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846973569
I just pushed a change to the new branch which got rid of the segfault while not double allocating memory for the complex-type case but only for optional fields with simple types ... please have a look if that's a good compromise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846937249
Aaah ok... Seems there are some conflicts... I'll have a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] thomas169 commented on pull request #244: plc4c: memory plumbing
Posted by GitBox <gi...@apache.org>.
thomas169 commented on pull request #244:
URL: https://github.com/apache/plc4x/pull/244#issuecomment-846964361
Okay the aim is to comment out optional field allocs in copt_packet.c and s7_message.c as they get double alloc'd.
If other optional field allocs need to stay then I guess some slightly bigger code-gen changes will be needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org