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

For s390x arch the auto generated go object's name needs to have s390x prefix #1308

Closed
wants to merge 1 commit into from

Conversation

msherif1234
Copy link

@msherif1234 msherif1234 commented Jan 15, 2024

s390 is not a valid GOARCH changing yje auto generated go file to use s390x arch instead.
fix #1307

@msherif1234 msherif1234 requested a review from a team as a code owner January 15, 2024 20:21
@msherif1234 msherif1234 changed the title for s390x arch the auto generated objects need to have s390x prefix for s390x arch the auto generated go object name needs to have s390x prefix Jan 15, 2024
@msherif1234 msherif1234 changed the title for s390x arch the auto generated go object name needs to have s390x prefix For s390x arch the auto generated go object name needs to have s390x prefix Jan 15, 2024
@msherif1234 msherif1234 changed the title For s390x arch the auto generated go object name needs to have s390x prefix For s390x arch the auto generated go object's name needs to have s390x prefix Jan 15, 2024
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Can you try simply passing -target s390x instead of -target s390? I think that will do the right thing. Ignore the file name, the important bit is the build constraint contained within.

@@ -66,7 +66,7 @@ var targetByGoArch = map[string]target{
"mips64p32": {"bpfeb", ""},
"ppc64": {"bpfeb", "powerpc"},
"s390": {"bpfeb", "s390"},
"s390x": {"bpfeb", "s390"},
"s390x": {"bpfeb", "s390x"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct since the second entry in target is used to define __TARGET_ARCH_s390, etc. There is no __TARGET_ARCH_s390x from what I can tell.

Copy link
Author

Choose a reason for hiding this comment

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

u mean the define in bpf_tracing.h right I think the same target _s390 seems to be used for both arch 32bits and 64bits at least that is what I saw in iovisor/bcc@7d7b6e8

Copy link
Author

Choose a reason for hiding this comment

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

u are right reworked the diffs PTAL

@msherif1234
Copy link
Author

Can you try simply passing -target s390x instead of -target s390? I think that will do the right thing. Ignore the file name, the important bit is the build constraint contained within.

using -target s390x also generate files with _390.* which fail go build

@lmb
Copy link
Collaborator

lmb commented Jan 16, 2024

Can you copy and paste the error you get when using -target s390x please?

@msherif1234
Copy link
Author

Can you copy and paste the error you get when using -target s390x please?

the auto generated go file not getting embedded when build so GOARCH=s390x because the generated file is _s390.go instead of _s390x.go

GOARCH=s390x  make compile
### Compiling project
# github.com/netobserv/netobserv-ebpf-agent/pkg/ebpf
pkg/ebpf/tracer.go:55:22: undefined: BpfObjects
pkg/ebpf/tracer.go:84:15: undefined: LoadBpf
pkg/ebpf/tracer.go:401:48: undefined: BpfFlowId
pkg/ebpf/tracer.go:401:60: undefined: BpfFlowMetrics
pkg/ebpf/tracer.go:405:23: undefined: BpfFlowId
pkg/ebpf/tracer.go:405:35: undefined: BpfFlowMetrics
pkg/ebpf/tracer.go:406:9: undefined: BpfFlowId
pkg/ebpf/tracer.go:407:16: undefined: BpfFlowMetrics
pkg/ebpf/tracer.go:452:78: undefined: BpfObjects
pkg/ebpf/tracer.go:512:18: undefined: BpfObjects
pkg/ebpf/tracer.go:407:16: too many errors
make: *** [Makefile:131: compile] Error 1

I shared the build logs in issue #1307

@lmb
Copy link
Collaborator

lmb commented Jan 16, 2024

Okay, in that case the issue is that s390 is a valid build tag. The go toolchain interprets _s390.go as an additional constraint to satisfy, which of course doesn't work. Oh boy.

@msherif1234
Copy link
Author

msherif1234 commented Jan 16, 2024

Yes GO doesn't consider s390 as vaild GOARCH https://go.dev/doc/install/source#environment
that is why in my changes I tried to replace _s390.* extenstion with _s390x.* to get things working, but it seems I was wrong assuming I can handle this bpf2go ? do u have any suggestions to fix this ?

@florianl
Copy link
Contributor

Just my thought looking into this a bit more:
The resulting Go filename is created by the following statement:

stem := fmt.Sprintf("%s_%s", outputStem, tgt.clang)

Here the element clang is used from the struct target which fits most use cases. But for GOARCH s390x it doesn't work, as clang can't handle the suffix x and so there is a difference between clangs __TARGET_ARCH_xzy and GOARCH.

At the moment, I think, it is not possible to determine GOARCH from the value combinations of target. So maybe one simple approach might be to extend the target struct with the GOARCH information and adapt creation of goFileName via stem accordingly.

@msherif1234 msherif1234 force-pushed the s390x-tgt branch 2 times, most recently from b384e80 to 26e3445 Compare January 16, 2024 20:51
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
@lmb
Copy link
Collaborator

lmb commented Jan 17, 2024

@florianl take a look at 00aa2c3 which hopefully clarifies goarch vs clang target vs linux target. The way files are generated are as follows:

  • The file name suffix is _$linux_$clang.go or _$clang.go. This is because any (clang, linux) target tuple can have multiple GOARCH associated with it.
  • The actual build constraints are embedded in the file using // go:build since that is more flexible than just the file suffix tag that the Go toolchain also supports.

The problem is that the clang target s390 is also a valid GOARCH, and the toolchain interprets the filename as adding a build constraint on GOARCH=s390. This of course fails.

I think the fix is to change the way we name files, which unfortunately is a breaking change. I'll try to prep a PR.

@msherif1234
Copy link
Author

Thanks @lmb pls cc me in your new PR

@msherif1234
Copy link
Author

this PR no longer needed a new PR by @lmb will be generated to fix this issue

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.

bpf2go issues with s390x arch
3 participants