On Fri, Sep 15, 2023 at 1:28 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote: > > On Thu, Sep 14, 2023 at 11:14:19AM -0700, Andrii Nakryiko wrote: > > On Thu, Sep 14, 2023 at 2:52 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote: > > > > > > On Thu, Sep 14, 2023 at 05:30:51PM +0900, Masahiro Yamada wrote: > > > > > > SNIP > > > > > > > > > so the change is about adding unique id that's basically path of > > > > > > the object stored in base32 so it could be used as symbol, so we > > > > > > don't really need to read the actual file > > > > > > > > > > > > the problem is when BTF_ID definition like: > > > > > > > > > > > > BTF_ID(struct, cgroup) > > > > > > > > > > > > translates in 2 separate objects into same symbol name because of > > > > > > the matching __COUNTER__ macro values (like 380 below) > > > > > > > > > > > > __BTF_ID__struct__cgroup__380 > > > > > > > > > > > > this change just adds unique id of the path name at the end of the > > > > > > symbol with: > > > > > > > > > > > > echo -n 'kernel/bpf/helpers' | base32 -w0 --> NNSXE3TFNQXWE4DGF5UGK3DQMVZHG > > > > > > > > > > > > so the symbol looks like: > > > > > > > > > > > > __BTF_ID__struct__cgroup__380NNSXE3TFNQXWE4DGF5UGK3DQMVZHG > > > > > > > > > > > > and is unique over the sources > > > > > > > > > > > > but I still hope we could come up with some better solution ;-) > > > > > > > > > > so far the only better solution I could come up with is to use > > > > > cksum (also from coreutils) instead of base32, which makes the > > > > > BTF_ID_BASE value compact > > > > > > > > > > I'll run test to find out how much it hurts the build time > > > > > > > > > > jirka > > > > > > > > > > > > > > > > Seems a bad idea to me. > > > > > > > > It would fork a new shell and chsum for all files, > > > > while only a few of them need it. > > > > > > right, I have a change to limit this on kernel and net directories, > > > but it's still bad > > > > > > > > > > > Better to consult BTF forks. > > > > > > perhaps there's better way within kbuild to get unique id/value > > > for each object file? > > > > let's just use __LINE__ + __COUNTER__ for now and teach resolve_btfids > > to fail and complain loudly about duplicate symbols? > > ok, will send that.. but it fails during link before resolve_btfids > takes place > > > > > > > This will give us time and opportunity to implement a better approach > > to .BTF_ids overall. Encoding the desired type name in the symbol name > > always felt off. Maybe it's better to encode type + name as data, > > which is discarded at the latest stage during vmlinux linking? Either > > hum, so maybe having a special section (.BTF_ids_desc below) > that would have record for each ID placed in .BTF_ids section: > > asm( \ > ".pushsection .BTF_ids,\"a\"; \n" \ > "1: \n" \ > ".zero 4 \n" \ > ".popsection; \n" \ > ".pushsection .BTF_ids_desc,\"a\"; \n" \ > ".long 1b \n" \ > > and somehow get prefix and name pointers in here: > > ".long prefix > ".long name > > ".popsection; \n"); > > so resolve_btfids would iterate .BTF_ids_desc records and fix > up .BTF_ids data.. > > we might need to do one extra link phase to get rid of the > .BTF_ids_desc secion It should be ok to keep it in vmlinux as we do for DWARF debug info. We'd want to make sure it's not retained in the compressed image though. Pretty sure `strip` is used to drop DWARF from the compressed image, but `strip` isn't going to recognize the semantics of some new ad-hoc ELF section. Pretty sure `objcopy` can be used to drop ELF sections; we'd need to probably invoke `objcopy` explicitly to drop that section (or add any new section to any pre-existing list of sections to drop). > > > way, this baseid hack seems worse and unnecessary. > > yes, it's bad > > jirka -- Thanks, ~Nick Desaulniers
