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

[infra] Update protobuf to 3.20 #14535

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

tomdol
Copy link
Contributor

@tomdol tomdol commented Jan 9, 2025

This commit updates the Protobuf library to a newer version due to the fixes this version contains.

In particular this version fixes some compilation errors being reported by GCC on Ubuntu 24.04
when the build is performed with the following flags enabled: -Werror -Wall

The removed .patch file is not required any more since the js_embed binary has been removed from protobuf
in the following pull request protocolbuffers/protobuf#4709

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak [email protected]

hseok-oh and others added 4 commits December 12, 2024 12:34
This commit adds onert on-device compiler document.

ONE-DCO-1.0-Signed-off-by: Hyeongseok Oh <[email protected]>
This commit updates the Protobuf library to a newer version due to the fixes this version contains.

In particular this version fixes some compilation errors being reported by GCC on Ubuntu 24.04
when the build is performed with the following flags enabled: -Werror -Wall

The removed .patch file is not required any more since the js_embed binary has been removed from protobuf
in the following pull request protocolbuffers/protobuf#4709

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak <[email protected]>
@seanshpark
Copy link
Contributor

seanshpark commented Jan 9, 2025

  • please check ARM32 build and share the results
  • please check with -DENABLE_EXCLUDE_ME=OFF so that inactive modules are also OK with this change
    • nncc configure -DENABLE_EXCLUDE_ME=OFF
      
    • PR builds with -DENABLE_EXCLUDE_ME=ON
    • there is weekly build with -DENABLE_EXCLUDE_ME=OFF but we don't want to wait for this

@tomdol
Copy link
Contributor Author

tomdol commented Jan 13, 2025

I was able to successfully build the project for ARM32 but by examining the build tree I was not able to find anything related to Protobuf. The source files are not even downloaded during the configuration or build phase.

I've used the instructions from the docs directory and the build was done using CROSS_BUILD=1 TARGET_ARCH=armv7l make -f Makefile.template

Did I miss anything? Could you please point me to the ARM32 CI jobs configuration so that I could compare the actual commands executed there?

@dahlinPL dahlinPL mentioned this pull request Jan 13, 2025
@seanshpark
Copy link
Contributor

CROSS_BUILD=1 TARGET_ARCH=armv7l make -f Makefile.template

You've been building onert not compiler.

Plz check https://github.com/Samsung/ONE/blob/master/docs/howto/how-to-build-compiler.md#cross-build-for-ubuntuarm32-experimental
And plz leave some logs about the results, not just "it's OK" sentence.

@seanshpark
Copy link
Contributor

seanshpark commented Jan 13, 2025

errors being reported by GCC on Ubuntu 24.04

plz paste the error log. just "there was error" doesn't help.

next time plz post an issue first describing the problem as the issue may have changes across modules.
current compiler PRs don't permit PRs across multiple modules, that is ONE/compiler/(module).

@seanshpark
Copy link
Contributor

@tomdol , I'm confused. your problem is with onert? or compiler?

@tomdol
Copy link
Contributor Author

tomdol commented Jan 14, 2025

plz leave some logs about the results, not just "it's OK" sentence.

My last comment was not the final report about the results of the cross compilation attempts. There was a statement that I couldn't find anything related to protobuf in the build tree so I thought there was no point in pasting any logs because they wouldn't prove anything.

A part of the compiler was built in the process though and this is the list of directories created by cmake:

~/projects/one/Product_arm32/armv7l-linux.debug/nncc/compiler [protobuf_update] $ ll
Jan 13 15:16 ./
Jan 13 15:18 ../
Jan 13 15:16 angkor/
Jan 13 15:16 CMakeFiles/
Jan 13 15:16 cmake_install.cmake
Jan 13 15:16 compiler.deps
Jan 13 15:16 CTestTestfile.cmake
Jan 13 15:16 foder/
Jan 13 15:16 hermes/
Jan 13 15:16 hermes-std/
Jan 13 15:16 loco/
Jan 13 15:16 locop/
Jan 13 15:16 logo/
Jan 13 15:16 logo-core/
Jan 13 15:16 luci/
Jan 13 15:16 luci-compute/
Jan 13 15:16 Makefile
Jan 13 15:16 mio-circle08/
Jan 13 15:16 oops/
Jan 13 15:16 pepper-csv2vec/
Jan 13 15:16 pepper-str/
Jan 13 15:16 pepper-strcast/
Jan 13 15:16 pp/

It looks like none of them depend on protobuf and that's why it wasn't downloaded nor compiled.

plz paste the error log. just "there was error" doesn't help.

An example full log can be accessed here https://github.sec.samsung.net/AIP/NPU_Compiler/actions/runs/14912799/job/45144450 although it contains multiple other errors that we're trying to gradually fix. An example warning (treated as error in that workflow's configuration) is:

/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:33: warning: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' reading between 1 and 2147483647 bytes from a region of size 0 [-Wstringop-overread]
   29 |   return __builtin___memcpy_chk (__dest, __src, __len,
      |          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   30 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
In member function 'void google::protobuf::RepeatedField<Element>::Reserve(int) [with Element = bool]':
cc1plus: note: source object is likely at address zero

Those warnings have been fixed in protobuf itself but the corrected code is only available in the newer releases.

I'm confused. your problem is with onert? or compiler?

The problem occurs when trying to build the NPU Compiler on Ubuntu 24.04 with the extra compiler flags. This is what we've been trying to address and since protobuf is not a direct dependency of the NPU Compiler, I'm trying to update it to solve one of a few steps of enabling the work on that project using the current LTS. The NPU Compiler depends on the compiler part of the ONE project but since I don't know all internal dependencies in ONE and all use cases where Protobuf is involved, I've submitted this PR and hoped for some suggestions about what I should build and/or run to validate the upgrade of PB. In particular I was wondering if there are any workflows/jobs that span both the compiler and onert that could potentially be affected by the upgrade of this dependency.

Thanks for the current suggestions, I will keep digging and will definitely share more detailed results as soon as I have them :)

@seanshpark
Copy link
Contributor

Q) why did you cross compile check with runtime?

@tomdol
Copy link
Contributor Author

tomdol commented Jan 14, 2025

Q) why did you cross compile check with runtime?

Just trying to figure out all dependencies and possible impact of the upgraded protobuf on the ONE project. I should have focused on the compiler first though, I guess.

@hseok-oh
Copy link
Contributor

@tomdol protobuf is not used in runtime build, so you can ignore effect to runtime on protobuf version up.

@tomdol
Copy link
Contributor Author

tomdol commented Jan 15, 2025

I have tested the compiler builds and here are my results:

ARM32 cross-compilation

I was able to successfully build the project with the following commands:

$ make -f infra/nncc/Makefile.arm32 cfg
$ make -f infra/nncc/Makefile.arm32 debug

Although I had to apply a minor change because of an error reported by the compiler:

error: comparison of unsigned expression in ‘< 0’ is always false [-Werror=type-limits]

The error comes from https://github.com/Samsung/ONE/blob/master/compiler/luci/lang/include/luci/IR/CircleNodeMixins.h#L76 and is also reported for the line number 91. It might be a compiler bug and it has already been reported by multiple users for multiple versions of GCC.

In the CircleNodeMixins.h file I have however added a compile-time check which discards both for loops for the template instances where N == 0. This removes the compilation error. Additionally the for loops are not compiled-in for the zero-arity nodes which (I think) makes sense given that such nodes don't have any inputs.

In the end I got 2 directories under build containing binaries for the host and ARM32:

   4096 Jan 14 13:56 ./
   4096 Jan 14 13:57 ../
2723576 Jan 14 13:47 flatc*
2558636 Jan 14 13:56 gif2h5*
2553384 Jan 14 13:56 h52gif*
2498560 Jan 14 13:56 h5copy*
2417728 Jan 14 13:55 h5debug*
2571800 Jan 14 13:55 h5diff*
2583380 Jan 14 13:56 h5dump*
2544836 Jan 14 13:56 h5import*
2498852 Jan 14 13:56 h5jam*
2534760 Jan 14 13:55 h5ls*
2498488 Jan 14 13:56 h5mkgrp*
2538260 Jan 14 13:56 h5repack*
2391440 Jan 14 13:55 h5repart*
2512456 Jan 14 13:56 h5stat*
2498676 Jan 14 13:56 h5unjam*
     15 Jan 14 13:51 protoc -> protoc-3.20.1.0*
3711200 Jan 14 13:51 protoc-3.20.1.0*

~/projects/one/build/arm32.debug/overlay/bin [protobuf_update] $ file protoc-3.20.1.0
protoc-3.20.1.0: ELF 32-bit LSB pie executable, ARM, EABI5 version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, BuildID[sha1]=e2bdfaf4d3c0931982875301fc885e877ec37659, for GNU/Linux 3.2.0, not stripped

A build with -DENABLE_EXCLUDE_ME=OFF

This build eventually also ended successfully but I've discovered a change in the Protobuf's API that required a small change in the compiler/mir module. At some point Protobuf has deprecated and finally removed a 2-arguments version of CodedInputStream::SetTotalBytesLimit() method:

  PROTOBUF_RUNTIME_DEPRECATED(
      "Please use the single parameter version of SetTotalBytesLimit(). The "
      "second parameter is ignored.")
  void SetTotalBytesLimit(int total_bytes_limit, int) {
    SetTotalBytesLimit(total_bytes_limit);
  }

After adapting the 3 affected files in compiler/mir I got a successful build:

$ ./nncc configure -DENABLE_EXCLUDE_ME=OFF -DEXTERNALS_BUILD_THREADS=16
(...)
-- Build files have been written to: /home/t.dolbniak/projects/one/build
$ echo $?
0

$ ./nncc build -j16
(...)
[100%] Parition Part_Mul_Sqrt_FC_nobias_000.circle with Part_Mul_Sqrt_FC_nobias_000_001.part
[100%] Built target circle_part_value_py_test_prepare
[100%] Parition Part_Mul_Sqrt_FC_nobias_000.circle with Part_Mul_Sqrt_FC_nobias_000_002.part
[100%] Built target circle_part_value_test_prepare
[100%] Built target luci_partition_test
$ echo $?
0

Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

plz split PRs for each modules, compiler/*

@seanshpark
Copy link
Contributor

multiple versions of GCC.

what is your GCC version?

@tomdol
Copy link
Contributor Author

tomdol commented Jan 16, 2025

multiple versions of GCC.

what is your GCC version?

In my case it was 10.5.0:

$ /usr/bin/arm-linux-gnueabihf-g++ --version
arm-linux-gnueabihf-g++ (Ubuntu 10.5.0-1ubuntu1~22.04) 10.5.0

@tomdol tomdol changed the title [infra] Update protobuf to 3.20.1 [infra] Update protobuf to 3.20 Jan 16, 2025
@tomdol
Copy link
Contributor Author

tomdol commented Jan 16, 2025

I've changed the Protobuf version to 3.20.2 because 3.20.1 contains a potential security vulnerability GHSA-8gq9-2x98-w8hf

I've rebuilt everything from scratch and got the same results as yesterday.

@seanshpark
Copy link
Contributor

arm-linux-gnueabihf-g++ (Ubuntu 10.5.0-1ubuntu1~22.04) 10.5.0

I assumed you are using U24.04, with other issues in npu compiler.

I'm using U22.04 and got

$ arm-linux-gnueabihf-g++ --version
arm-linux-gnueabihf-g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

strange... what is your Ubuntu version?
how did you installed arm-linux-gnueabihf-g++ ?
have you tried with higher version of arm-linux-gnueabihf-g++ ?

@dahlinPL
Copy link
Contributor

dahlinPL commented Jan 16, 2025

arm-linux-gnueabihf-g++ (Ubuntu 10.5.0-1ubuntu1~22.04) 10.5.0

I assumed you are using U24.04, with other issues in npu compiler.

I'm using U22.04 and got

$ arm-linux-gnueabihf-g++ --version
arm-linux-gnueabihf-g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

strange... what is your Ubuntu version? how did you installed arm-linux-gnueabihf-g++ ? have you tried with higher version of arm-linux-gnueabihf-g++ ?

@seanshpark - it's not determined which cross compiler should be used in Ubuntu versions newer that 20.04, and for 20.04 it's 9 or 10 (https://github.com/Samsung/ONE/blob/master/docs/howto/how-to-cross-build-runtime-for-arm.md#install-arm-cross-toolchain). In your opinion we should use some specific version of arm-linux-gnueabihf-g++ ? (AFAIK @tomdol has Ubuntu 22.04, I have 24,04, I was able to build project with v10.5.0, I've installed it since it was version pointed in docs)

@tomdol
Copy link
Contributor Author

tomdol commented Jan 16, 2025

I have just finished testing with GCC 9 and GCC 11, both on Ubuntu 22.04. This time it compiled without the error so perhaps GCC 10.5 has some specific problem.

Anyway I've removed the related change from this PR and only left the code adjustments required by the Protobuf's API change.

@seanshpark
Copy link
Contributor

so perhaps GCC 10.5 has some specific problem.

OK, that's good.
If possible, it would be nice to add a NOTE in build how to document about this.

@tomdol
Copy link
Contributor Author

tomdol commented Jan 17, 2025

Do you mean a note about the workaround(with if-constexpr) if someone encounters this build error on GCC 10? And would it be ok to add such information in a section like "Known issues" at the end of this document? https://github.com/Samsung/ONE/blob/master/docs/howto/how-to-build-compiler.md#cross-build-for-ubuntuarm32-experimental

@seanshpark
Copy link
Contributor

if someone encounters this build error on GCC 10?

I think we recommend to use g++ version >= 11 would be OK cause of the issue "something something".

like "Known issues" at the end of this document?

yes, some explanation about g++ == 10 can give issue "something something..."

@tomdol
Copy link
Contributor Author

tomdol commented Jan 17, 2025

I've added the suggested description to the docs, please have a look if this wording makes sense.

@tomdol tomdol requested a review from seanshpark January 17, 2025 10:12
@@ -208,3 +208,31 @@ NOTE: this assumes
- host and target have same directoy structure
- should copy `build` folder to target or
- mounting `ONE` folder with NFS on the target would be simple

## Known issues
There's a potential known build error when attempting to cross-compile for ARM32 using GCC 10.5. You might encounter an error `comparison of unsigned expression in ‘< 0’ is always false [-Werror=type-limits]` reported from the `CircleNodeMixins.h` file. This is likely GCC's bug in this specific version. There's a workaround for it though - you can apply the following changes to both for loops in the `CircleNodeMixins.h`:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please split lines within 100 cols for not h-scrolling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, already done

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.

4 participants