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.

Sign up for GitHub

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

Jump to bottom

Fix compilation error after Trace Compass API clean-up #28

Merged
merged 1 commit into from
May 28, 2024
Merged

Fix compilation error after Trace Compass API clean-up #28

merged 1 commit into from
May 28, 2024

Conversation

bhufmann
Copy link
Contributor

@bhufmann bhufmann commented May 23, 2024 •
edited
Loading

See eclipse-tracecompass/org.eclipse.tracecompass#87 for API clean-up.

Note: XAF was broken before due incompatible changes in mainline TC. This PR just makes it compile again, but it's still broken.

Signed-off-by: Bernd Hufmann bernd.hufmann@ericsson.com

@bhufmann bhufmann force-pushed the fix-error-after-api-clean-up branch from 46b8ac2 to 40ef72d Compare May 23, 2024 14:44
bhufmann
bhufmann commented May 23, 2024
View reviewed changes
.../tracecompass/incubator/internal/lttng2/ust/extras/core/pthread/PThreadLockGraphHandler.java
// First add a vertex at the time of lock request
graph.append(worker, lastReqVertex, EdgeType.RUNNING);
graph.append(lastReqVertex, new OSEdgeContextState(OSEdgeContextEnum.RUNNING));
Copy link
Contributor Author

@bhufmann bhufmann May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OSEdgeConstextState and OSEdgeContextEnum are internal. Should they be public API or is there a different (more correct) way to create ITmfEdgeContextState here? @MatthewKhouzam what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed there should. I would suggest that's a different patch, for now, we'll grin and bare it.

.../tracecompass/incubator/internal/lttng2/ust/extras/core/pthread/PThreadLockGraphHandler.java
lastOwner.fVertex.linkVertical(unblockVertex);
// TODO: check if it's the correct replacement
// lastOwner.fVertex.linkVertical(unblockVertex);
graph.edgeVertical(lastOwner.fVertex, unblockVertex, new OSEdgeContextState(OSEdgeContextEnum.DEFAULT), MUTEX_FIELD);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is a correct replacement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks about right to me. @arfio please confirm!

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good to me also.

@bhufmann bhufmann force-pushed the fix-error-after-api-clean-up branch from 40ef72d to 7463ae2 Compare May 23, 2024 15:15
@bhufmann bhufmann requested a review from MatthewKhouzam May 24, 2024 18:07
MatthewKhouzam
MatthewKhouzam requested changes May 27, 2024
View reviewed changes
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

Minor changes.

.../tracecompass/incubator/internal/lttng2/ust/extras/core/pthread/PThreadLockGraphHandler.java
// First add a vertex at the time of lock request
graph.append(worker, lastReqVertex, EdgeType.RUNNING);
graph.append(lastReqVertex, new OSEdgeContextState(OSEdgeContextEnum.RUNNING));
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed there should. I would suggest that's a different patch, for now, we'll grin and bare it.

.../tracecompass/incubator/internal/lttng2/ust/extras/core/pthread/PThreadLockGraphHandler.java
lastOwner.fVertex.linkVertical(unblockVertex);
// TODO: check if it's the correct replacement
// lastOwner.fVertex.linkVertical(unblockVertex);
graph.edgeVertical(lastOwner.fVertex, unblockVertex, new OSEdgeContextState(OSEdgeContextEnum.DEFAULT), MUTEX_FIELD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks about right to me. @arfio please confirm!

...ecompass/incubator/internal/xaf/core/statemachine/variable/StateMachineVariableAnalysis.java Outdated Show resolved Hide resolved
...ecompass/incubator/internal/xaf/core/statemachine/variable/StateMachineVariableAnalysis.java Outdated Show resolved Hide resolved
@bhufmann bhufmann force-pushed the fix-error-after-api-clean-up branch from 7463ae2 to d789de3 Compare May 27, 2024 16:21
@bhufmann bhufmann requested review from MatthewKhouzam and arfio May 27, 2024 16:21
arfio
arfio requested changes May 27, 2024
View reviewed changes
.../tracecompass/incubator/internal/lttng2/ust/extras/core/pthread/PThreadLockGraphHandler.java
lastOwner.fVertex.linkVertical(unblockVertex);
// TODO: check if it's the correct replacement
// lastOwner.fVertex.linkVertical(unblockVertex);
graph.edgeVertical(lastOwner.fVertex, unblockVertex, new OSEdgeContextState(OSEdgeContextEnum.DEFAULT), MUTEX_FIELD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good to me also.

...ecompass/incubator/internal/xaf/core/statemachine/variable/StateMachineVariableAnalysis.java Outdated
// So, the implementation was broken and this change just makes it compile

// TmfGraph path = new CriticalPathAlgorithmBounded(graph).compute(Objects.requireNonNull(vstart), Objects.requireNonNull(vend));
ITmfGraph path = graph;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be fixed using the OSCriticalPathAlgorithm instead. I think it should be one line:
TmfGraph path = new OSCriticalPathAlgorithmBounded(graph).compute(Objects.requireNonNull(vstart), Objects.requireNonNull(vend));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I figured it out

...ecompass/incubator/internal/xaf/core/statemachine/variable/StateMachineVariableAnalysis.java Show resolved Hide resolved
...ecompass/incubator/internal/xaf/core/statemachine/variable/StateMachineVariableAnalysis.java Show resolved Hide resolved
.../tracecompass/incubator/internal/xaf/core/statemachine/variable/utils/CriticalPathState.java Show resolved Hide resolved
@bhufmann
Fix compilation error after Trace Compass API clean-up
d249426 
See
eclipse-tracecompass/org.eclipse.tracecompass#87

Note: XAF was broken before due incompatible changes in mainline TC.
This commit, just makes it compile again, but it's still broken.

Signed-off-by: Bernd Hufmann <bernd.hufmann@ericsson.com>
@bhufmann bhufmann force-pushed the fix-error-after-api-clean-up branch from d789de3 to d249426 Compare May 28, 2024 13:37
@bhufmann bhufmann requested a review from arfio May 28, 2024 13:44
MatthewKhouzam
MatthewKhouzam approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

I am fine, we know this is a "stopgap" patch

@arfio
Copy link
Contributor

arfio commented May 28, 2024

I think we should make sure that it is using the same set of classes and not a mix of the old and new ones that will create issues later (at least in the same file).

@bhufmann
Copy link
Contributor Author

I think we should make sure that it is using the same set of classes and not a mix of the old and new ones that will create issues later (at least in the same file).

@MatthewKhouzam could you please confirm that you can address that in your follow-up PR?

@bhufmann bhufmann mentioned this pull request May 28, 2024
Merged
@MatthewKhouzam
Copy link
Contributor

I will share the WIP soon. This is being done on a "volunteer" basis though. We need to get the build going soon.

@bhufmann bhufmann merged commit e4dfbac into eclipse-tracecompass-incubator:master May 28, 2024
2 checks passed
@bhufmann bhufmann deleted the fix-error-after-api-clean-up branch May 28, 2024 14:36
@bhufmann bhufmann mentioned this pull request May 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arfio arfio arfio approved these changes

@MatthewKhouzam MatthewKhouzam MatthewKhouzam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bhufmann @arfio @MatthewKhouzam