| Message ID | 20220927221133.594071-1-f.fainelli@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | package/linux-tools: Exclude checking PE binaries from perf test | expand |
Hello Florian, On Tue, 27 Sep 2022 15:11:33 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: > Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 > ("perf tests: Add test for PE binary format support") present in >= > v5.10 there is an unconditional installation of PE binaries which will > be rejected by the check-bin-arch script. > > Make sure that these binaries are excluded from being checked to allow > the installation of the perf tests. > > Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Thanks for the patch! Before merging it, I'd like to understand a little bit more how readelf behaves for these PE files. Indeed in check-bin-arch, we are doing: arch=$(LC_ALL=C ${readelf} -h "${TARGET_DIR}/${f}" 2>&1 | \ sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1) # If no architecture found, assume it was not an ELF file if test "${arch}" = "" ; then continue fi for a PE file, I would expect readelf to badly fail, and therefore ${arch} to be empty, and the file simply ignored. What is the behavior/output of readelf on these PE files? Thanks! Thomas
On 9/28/22 14:38, Thomas Petazzoni wrote: > Hello Florian, > > On Tue, 27 Sep 2022 15:11:33 -0700 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 >> ("perf tests: Add test for PE binary format support") present in >= >> v5.10 there is an unconditional installation of PE binaries which will >> be rejected by the check-bin-arch script. >> >> Make sure that these binaries are excluded from being checked to allow >> the installation of the perf tests. >> >> Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Thanks for the patch! Before merging it, I'd like to understand a > little bit more how readelf behaves for these PE files. Indeed in > check-bin-arch, we are doing: > > arch=$(LC_ALL=C ${readelf} -h "${TARGET_DIR}/${f}" 2>&1 | \ > sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1) > > # If no architecture found, assume it was not an ELF file > if test "${arch}" = "" ; then > continue > fi > > for a PE file, I would expect readelf to badly fail, and therefore > ${arch} to be empty, and the file simply ignored. > > What is the behavior/output of readelf on these PE files? If I use my host system readelf which is packaged from binutils-x86-64-linux-gnu, I get no output and all is well, however when check-bin-arch is called and it uses $(TARGET_READELF), I do get: ./host/bin/aarch64-linux-readelf -h build/linux-custom/tools/perf/tests/pe-file.exe | sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1 IMAGE_FILE_MACHINE_AMD64 (0x8664) which is what prompted me to issue this patch in the first place. I should mention that the readelf binary in this case is the LLVM Object Reader and it does support PE/COFF which is probably why it even remotely attempts to parse the file.
On Wed, 28 Sep 2022 15:28:58 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: > ./host/bin/aarch64-linux-readelf -h > build/linux-custom/tools/perf/tests/pe-file.exe | sed -r -e '/^ > Machine: +(.+)/!d; s//\1/;' | head -1 > IMAGE_FILE_MACHINE_AMD64 (0x8664) Could you provide the full output? In other words, I'm interested to see if it's really readelf showing the same "Machine:" field as for regular ELF files, or if it's something somewhat different that we could distinguish. > I should mention that the readelf binary in this case is the LLVM Object > Reader and it does support PE/COFF which is probably why it even > remotely attempts to parse the file. How come your readelf is from LLVM? Are you using an external toolchain that isn't based on the standard GNU Binutils? Thanks! Thomas
On 9/28/22 23:40, Thomas Petazzoni wrote: > On Wed, 28 Sep 2022 15:28:58 -0700 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> ./host/bin/aarch64-linux-readelf -h >> build/linux-custom/tools/perf/tests/pe-file.exe | sed -r -e '/^ >> Machine: +(.+)/!d; s//\1/;' | head -1 >> IMAGE_FILE_MACHINE_AMD64 (0x8664) > > Could you provide the full output? In other words, I'm interested to > see if it's really readelf showing the same "Machine:" field as for > regular ELF files, or if it's something somewhat different that we > could distinguish. Sure, please find attached. > >> I should mention that the readelf binary in this case is the LLVM Object >> Reader and it does support PE/COFF which is probably why it even >> remotely attempts to parse the file. > > How come your readelf is from LLVM? Are you using an external toolchain > that isn't based on the standard GNU Binutils? Correct, I am using a custom built LLVM toolchain that uses the Clang integrated assembler and none of the binutils binaries, or any GNU component for that matter.
Florian, All, On 2022-09-28 15:28 -0700, Florian Fainelli spake thusly: > On 9/28/22 14:38, Thomas Petazzoni wrote: > >On Tue, 27 Sep 2022 15:11:33 -0700 > >Florian Fainelli <f.fainelli@gmail.com> wrote: > >>Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 > >>("perf tests: Add test for PE binary format support") present in >= > >>v5.10 there is an unconditional installation of PE binaries which will > >>be rejected by the check-bin-arch script. > >> > >>Make sure that these binaries are excluded from being checked to allow > >>the installation of the perf tests. > >> > >>Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") > >>Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > >Thanks for the patch! Before merging it, I'd like to understand a > >little bit more how readelf behaves for these PE files. Indeed in > >check-bin-arch, we are doing: > > > > arch=$(LC_ALL=C ${readelf} -h "${TARGET_DIR}/${f}" 2>&1 | \ > > sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1) > > > > # If no architecture found, assume it was not an ELF file > > if test "${arch}" = "" ; then > > continue > > fi > > > >for a PE file, I would expect readelf to badly fail, and therefore > >${arch} to be empty, and the file simply ignored. > > > >What is the behavior/output of readelf on these PE files? > > If I use my host system readelf which is packaged from > binutils-x86-64-linux-gnu, I get no output and all is well, however when > check-bin-arch is called and it uses $(TARGET_READELF), I do get: > > ./host/bin/aarch64-linux-readelf -h > build/linux-custom/tools/perf/tests/pe-file.exe | sed -r -e '/^ Machine: > +(.+)/!d; s//\1/;' | head -1 > IMAGE_FILE_MACHINE_AMD64 (0x8664) > > which is what prompted me to issue this patch in the first place. > > I should mention that the readelf binary in this case is the LLVM Object > Reader and it does support PE/COFF which is probably why it even remotely > attempts to parse the file. OK, but isn't that flawed? It is named 'readelf', so I would expect it to not report on non-ELF files, at least by default (i.e. unless a specific option flag is passed). If a readelf tool does not behave like a readelf tool should behave, isn't that a bug in that specific readelf tool to begin with? Otherwise, it is very intriguing how you managed to have a toolchain without gcc et al, be usable for Buildroot that only knows about the GNU ecosystem. Is that a publicly available toolchain? Can you share the recipe how that toolchain was generated? Do you have special, local changes in Buildroot to accomodate that toolchain? Will you submit those changes? That would allow adding more testing to avoid such issue in the future. Otherwise, I am not fan of adding such exclusion for private cases that we can not (easily) reproduce, as those cases are not supported. Regards, Yann E. MORIN. > -- > Florian > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On 10/1/22 12:07, Yann E. MORIN wrote: > Florian, All, > > On 2022-09-28 15:28 -0700, Florian Fainelli spake thusly: >> On 9/28/22 14:38, Thomas Petazzoni wrote: >>> On Tue, 27 Sep 2022 15:11:33 -0700 >>> Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 >>>> ("perf tests: Add test for PE binary format support") present in >= >>>> v5.10 there is an unconditional installation of PE binaries which will >>>> be rejected by the check-bin-arch script. >>>> >>>> Make sure that these binaries are excluded from being checked to allow >>>> the installation of the perf tests. >>>> >>>> Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> >>> Thanks for the patch! Before merging it, I'd like to understand a >>> little bit more how readelf behaves for these PE files. Indeed in >>> check-bin-arch, we are doing: >>> >>> arch=$(LC_ALL=C ${readelf} -h "${TARGET_DIR}/${f}" 2>&1 | \ >>> sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1) >>> >>> # If no architecture found, assume it was not an ELF file >>> if test "${arch}" = "" ; then >>> continue >>> fi >>> >>> for a PE file, I would expect readelf to badly fail, and therefore >>> ${arch} to be empty, and the file simply ignored. >>> >>> What is the behavior/output of readelf on these PE files? >> >> If I use my host system readelf which is packaged from >> binutils-x86-64-linux-gnu, I get no output and all is well, however when >> check-bin-arch is called and it uses $(TARGET_READELF), I do get: >> >> ./host/bin/aarch64-linux-readelf -h >> build/linux-custom/tools/perf/tests/pe-file.exe | sed -r -e '/^ Machine: >> +(.+)/!d; s//\1/;' | head -1 >> IMAGE_FILE_MACHINE_AMD64 (0x8664) >> >> which is what prompted me to issue this patch in the first place. >> >> I should mention that the readelf binary in this case is the LLVM Object >> Reader and it does support PE/COFF which is probably why it even remotely >> attempts to parse the file. > > OK, but isn't that flawed? It is named 'readelf', so I would expect it > to not report on non-ELF files, at least by default (i.e. unless a > specific option flag is passed). > > If a readelf tool does not behave like a readelf tool should behave, > isn't that a bug in that specific readelf tool to begin with? Markus can correct me but it seems to me llvm-readelf is LLVM's own doing and aliases to llvm-readobj, given that PE executable support was built into the toolchain, it happily went into parsing the PE files. I suppose that in premise llvm-readelf should retrict itself to parsing ELF files, clearly LLVM developers thought differently. > > Otherwise, it is very intriguing how you managed to have a toolchain > without gcc et al, be usable for Buildroot that only knows about the GNU > ecosystem. > > Is that a publicly available toolchain? Not yet, but soon. > Can you share the recipe how that toolchain was generated? Yes that will be done. > Do you have special, local changes in Buildroot to accomodate that toolchain? Will you submit those changes? Yes a few and yes we will, provided there is interest from the buildroot community to have support for a GNU-free and musl-libc based toolchain. The approach we took for now is that we provide a GNU-compatible wrapper and set of executable names such that you build with clang/LLVM but you don't really need to know about it, except where it matters. > > That would allow adding more testing to avoid such issue in the future. > Otherwise, I am not fan of adding such exclusion for private cases that > we can not (easily) reproduce, as those cases are not supported. Fair enough, how about we make sure the toolchain is available, can be re-created by anyone who desires so and then we submit the changes to buildroot that we have so you see the full picture?
Florian, All, On 2022-10-03 10:21 -0700, Florian Fainelli spake thusly: > On 10/1/22 12:07, Yann E. MORIN wrote: > >On 2022-09-28 15:28 -0700, Florian Fainelli spake thusly: > >>On 9/28/22 14:38, Thomas Petazzoni wrote: > >>>On Tue, 27 Sep 2022 15:11:33 -0700 > >>>Florian Fainelli <f.fainelli@gmail.com> wrote: > >>>>Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 > >>>>("perf tests: Add test for PE binary format support") present in >= > >>>>v5.10 there is an unconditional installation of PE binaries which will > >>>>be rejected by the check-bin-arch script. > >>>> > >>>>Make sure that these binaries are excluded from being checked to allow > >>>>the installation of the perf tests. [--SNIP--] > >>I should mention that the readelf binary in this case is the LLVM Object > >>Reader and it does support PE/COFF which is probably why it even remotely > >>attempts to parse the file. > > > >Is that a publicly available toolchain? > >Can you share the recipe how that toolchain was generated? > Yes that will be done. Great! > >Do you have special, local changes in Buildroot to accomodate that toolchain? Will you submit those changes? > Yes a few and yes we will, provided there is interest from the buildroot > community to have support for a GNU-free and musl-libc based toolchain. I am personally not interested in a "GNU-free" toolchain. However, I am very much interested in an llvm-based toolchain. Semantics, semantics! ;-) > The > approach we took for now is that we provide a GNU-compatible wrapper and set > of executable names such that you build with clang/LLVM but you don't really > need to know about it, except where it matters. Yes, that sounds like a good approach. > >That would allow adding more testing to avoid such issue in the future. > >Otherwise, I am not fan of adding such exclusion for private cases that > >we can not (easily) reproduce, as those cases are not supported. > Fair enough, how about we make sure the toolchain is available, can be > re-created by anyone who desires so and then we submit the changes to > buildroot that we have so you see the full picture? Yes, that would be awesome! As I said above: having support for using an llvm-based toolchain would be very interesting! Regards, Yann E. MORIN.
Florian, All, On 2022-09-27 15:11 -0700, Florian Fainelli spake thusly: > Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 > ("perf tests: Add test for PE binary format support") present in >= > v5.10 there is an unconditional installation of PE binaries which will > be rejected by the check-bin-arch script. > > Make sure that these binaries are excluded from being checked to allow > the installation of the perf tests. > > Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> I've finally applied to master, thanks. However, I wonder if we could not be a bit more robust in check-bin-arch itself, something like: diff --git a/support/scripts/check-bin-arch b/support/scripts/check-bin-arch index 27cc59bca0..2920a95ae3 100755 --- a/support/scripts/check-bin-arch +++ b/support/scripts/check-bin-arch @@ -58,6 +58,9 @@ exitcode=0 IFS=" " +# ELF magic +ELF_MAGIC="$(printf "\x7f\x45\x4c\x46")" + while read f; do for ignore in "${IGNORES[@]}"; do if [[ "${f}" =~ ^"${ignore}" ]]; then @@ -71,13 +74,19 @@ while read f; do continue fi + # Check if it looks like it is an ELF file + read -N 4 magic <"${TARGET_DIR}/${f}" + if [ "${magic}" != "${ELF_MAGIC}" ]; then + continue + fi + # Get architecture using readelf. We pipe through 'head -1' so # that when the file is a static library (.a), we only take # into account the architecture of the first object file. arch=$(LC_ALL=C ${readelf} -h "${TARGET_DIR}/${f}" 2>&1 | \ sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1) -# If no architecture found, assume it was not an ELF file +# If no architecture found, assume it was in fact not an ELF file if test "${arch}" = "" ; then continue fi (with the read -N 4 trick as discussed in [0]) Thoughts? [0] https://lore.kernel.org/buildroot/10267_1663749711_632ACE4F_10267_83_8_20220921084149.GB2398274@tl-lnx-nyma7486/ Regards, Yann E. MORIN. > --- > package/linux-tools/linux-tool-perf.mk.in | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in > index dda63cccecb4..c22097316264 100644 > --- a/package/linux-tools/linux-tool-perf.mk.in > +++ b/package/linux-tools/linux-tool-perf.mk.in > @@ -169,6 +169,10 @@ define PERF_INSTALL_REMOVE_SCRIPTS > $(RM) -rf $(TARGET_DIR)/usr/libexec/perf-core/scripts/ > $(RM) -rf $(TARGET_DIR)/usr/libexec/perf-core/tests/ > endef > + > +LINUX_TOOLS_BIN_ARCH_EXCLUDE += \ > + /usr/libexec/perf-core/tests/pe-file.exe \ > + /usr/libexec/perf-core/tests/pe-file.exe.debug > endif > > define PERF_INSTALL_TARGET_CMDS > -- > 2.25.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On 4/17/2023 12:58 PM, Yann E. MORIN wrote: > Florian, All, > > On 2022-09-27 15:11 -0700, Florian Fainelli spake thusly: >> Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 >> ("perf tests: Add test for PE binary format support") present in >= >> v5.10 there is an unconditional installation of PE binaries which will >> be rejected by the check-bin-arch script. >> >> Make sure that these binaries are excluded from being checked to allow >> the installation of the perf tests. >> >> Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > I've finally applied to master, thanks. > > However, I wonder if we could not be a bit more robust in check-bin-arch > itself, something like: > > diff --git a/support/scripts/check-bin-arch b/support/scripts/check-bin-arch > index 27cc59bca0..2920a95ae3 100755 > --- a/support/scripts/check-bin-arch > +++ b/support/scripts/check-bin-arch > @@ -58,6 +58,9 @@ exitcode=0 > IFS=" > " > > +# ELF magic > +ELF_MAGIC="$(printf "\x7f\x45\x4c\x46")" > + > while read f; do > for ignore in "${IGNORES[@]}"; do > if [[ "${f}" =~ ^"${ignore}" ]]; then > @@ -71,13 +74,19 @@ while read f; do > continue > fi > > + # Check if it looks like it is an ELF file > + read -N 4 magic <"${TARGET_DIR}/${f}" > + if [ "${magic}" != "${ELF_MAGIC}" ]; then > + continue > + fi > + > # Get architecture using readelf. We pipe through 'head -1' so > # that when the file is a static library (.a), we only take > # into account the architecture of the first object file. > arch=$(LC_ALL=C ${readelf} -h "${TARGET_DIR}/${f}" 2>&1 | \ > sed -r -e '/^ Machine: +(.+)/!d; s//\1/;' | head -1) > > -# If no architecture found, assume it was not an ELF file > +# If no architecture found, assume it was in fact not an ELF file > if test "${arch}" = "" ; then > continue > fi > > (with the read -N 4 trick as discussed in [0]) > > Thoughts? That would work for me!
>>>>> "Florian" == Florian Fainelli <f.fainelli@gmail.com> writes: > Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 > ("perf tests: Add test for PE binary format support") present in >= > v5.10 there is an unconditional installation of PE binaries which will > be rejected by the check-bin-arch script. > Make sure that these binaries are excluded from being checked to allow > the installation of the perf tests. > Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Committed to 2023.02.x, thanks.
diff --git a/package/linux-tools/linux-tool-perf.mk.in b/package/linux-tools/linux-tool-perf.mk.in index dda63cccecb4..c22097316264 100644 --- a/package/linux-tools/linux-tool-perf.mk.in +++ b/package/linux-tools/linux-tool-perf.mk.in @@ -169,6 +169,10 @@ define PERF_INSTALL_REMOVE_SCRIPTS $(RM) -rf $(TARGET_DIR)/usr/libexec/perf-core/scripts/ $(RM) -rf $(TARGET_DIR)/usr/libexec/perf-core/tests/ endef + +LINUX_TOOLS_BIN_ARCH_EXCLUDE += \ + /usr/libexec/perf-core/tests/pe-file.exe \ + /usr/libexec/perf-core/tests/pe-file.exe.debug endif define PERF_INSTALL_TARGET_CMDS
Since upstream Linux commit ed21d6d7c48e6e96c2d617e304a7ebfbd17b1807 ("perf tests: Add test for PE binary format support") present in >= v5.10 there is an unconditional installation of PE binaries which will be rejected by the check-bin-arch script. Make sure that these binaries are excluded from being checked to allow the installation of the perf tests. Fixes: 6fcdaa4c5096 ("package/linux-tools: Allow installation of perf scripts") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- package/linux-tools/linux-tool-perf.mk.in | 4 ++++ 1 file changed, 4 insertions(+)