capstone-engine / capstone

Capstone disassembly/disassembler framework for ARM, ARM64 (ARMv8), Alpha, BPF, Ethereum VM, HPPA, LoongArch, M68K, M680X, Mips, MOS65XX, PPC, RISC-V(rv32G/rv64G), SH, Sparc, SystemZ, TMS320C64X, TriCore, Webassembly, XCore and X86.
http://www.capstone-engine.org
7.61k stars 1.56k forks source link

Fix pkgconfig file to make #include <capstone/capstone.h> work #2307

Closed ret2libc closed 6 months ago

ret2libc commented 7 months ago

Your checklist for this pull request

Detailed description

The simple example in https://www.capstone-engine.org/lang_c.html does not work for me because the it can't find capstone/capstone.h .

This is because the pkgconfig file only adds -I{includedir}/capstone and not -I{includedir}.

Test plan

Just try to compile the code in the documentation page.

Closing issues

...

XVilka commented 7 months ago

Note, that this is controversial change that was done back and forth mutliple times in the past. So it should be checked carefully first why it was done. cc @kabeor @Rot127

Rot127 commented 7 months ago

Please someone correct me if I am wrong. But don't we, without this change, get conflicts with other package's headers of the same name? Basically like https://github.com/capstone-engine/capstone/issues/2316?

@aquynh @kabeor Any comments?

ret2libc commented 7 months ago

Yes indeed the thing is that "recently" it was made "mandatory" to use #include <capstone/xyz.h> but then the pkgconfig file does not actually produce the right cflags to make that work.

Rot127 commented 7 months ago

Any reference to "recently" and the "mandatory" points? Because we should add the explanation to the commit message. So the discussion is decided once and for all :)

ret2libc commented 7 months ago

Any reference to "recently" and the "mandatory" points? Because we should add the explanation to the commit message. So the discussion is decided once and for all :)

I don't have any, sorry :D Just memory, so I could very well be wrong

Rot127 commented 7 months ago

I would like to test the collision theory before merging this. So we can say with absolute authority that this is the only way to do it, from now on. @ret2libc Would you mind doing this?

@grahamwoodward Can you test this one as well for you please?

ret2libc commented 7 months ago

I think it's slightly different from the collision you referenced. The pkg-config cflags are used when another package wants to depend on capstone (e.g. Rizin). AFAIR rizin fails to compile now with system capstone.

grahamwoodward commented 7 months ago

@Rot127 @ret2libc so I tried this single line change, applying it to my local check out of capstone.pc.in and then ran

sudo CAPSTONE_ARCHS="arm aarch64" ./make.sh uninstall
sudo make clean
sudo CAPSTONE_ARCHS="arm aarch64" ./make.sh install

then tried to recompile my BPF tool but it hit the same issue...

/usr/include/capstone/bpf.h:94:9: error: use of 'bpf_insn' with tag type that does not match previous declaration
typedef enum bpf_insn {
        ^
/home/grawoo02/tool/include/bpf/libbpf.h:334:8: note: previous use is here
struct bpf_insn;
ret2libc commented 7 months ago

I think that's a different problem from what I'm trying to solve?!

Why is capstone defining a enumerated with a prefix that is not cs_ ?

Rot127 commented 7 months ago

Why is capstone defining a enumerated with a prefix that is not cs_ ?

No idea. But it is the same pattern in all header files. Changing it is even for me a bit too much API change though :D

Anyways, I prefer your change over the previous version. Pinged @aquynh and @kabeor already and asked for their opinion.

grahamwoodward commented 7 months ago

I thought the fix was going to be (I did this in my local world) to change the enum bpf_insn to cs_bpf_insn...

grahamwoodward commented 7 months ago

Why is capstone defining a enumerated with a prefix that is not cs_ ?

No idea. But it is the same pattern in all header files. Changing it is even for me a bit too much API change though :D

Anyways, I prefer your change over the previous version. Pinged @aquynh and @kabeor already and asked for their opinion.

@Rot127 should I try with the first commit you asked me to try? I've only tried this commit so far

aquynh commented 7 months ago

ok so we just need to update the docs on the website, right?

ret2libc commented 7 months ago

ok so we just need to update the docs on the website, right?

No, the doc on the website uses capstone/capstone.h which is correct, but it does not use pkgconfig, so this bug is not hit. However people using libraries usually rely on pkg-config or cmake files to get the right cflags/libs.

XVilka commented 6 months ago

@kabeor @aquynh, what is your opinion on this one? It's better to merge this early to allow a few months of testing on various systems/distributions so it wouldn't make a surprise before the 6.0 release

kabeor commented 6 months ago

Thank you!