Further Makefile Improvements#13
Conversation
|
how would these "merged.o" files fit or "synergize" with the static lib target that is already there? |
|
This is a followup to the symbol hiding work we did earlier. The "merged localized" object file has private symbols set to LOCAL HIDDEN (as opposed to GLOBAL HIDDEN). If we pack the localized object file into an archive, that archive is ready for client-facing use cases. Downstream clients cannot link to the "localized" private functions by accident. The "merged" object file is a temporary side product. GNU's There is a blocker that prevented me from converting these merged objects into static archives. The tests makefile would pass |
|
Link to the Makefile Lines 39 to 40 in d1e9bde |
|
i do get this. and i very much appreciate this. what i'm missing, is how is this merged localized obj file related to the static archive. should this object file not replace the static archive after all? |
|
The merged-localized object does not expose any global hidden functions whereas the current Visibility flags in archived objects are interpreted differently than the dynamic linking case. This is partly because I have a demo here: vis.h// vis.h
#pragma once
#include <stdint.h>
int check_timestamp(void);
uint8_t* get_key(void);vis_test.c// vis_test.c
#include <stdint.h>
#include <stdlib.h>
#include "vis.h"
[[gnu::visibility("hidden")]] int check_timestamp(void) {
return -2;
};
[[gnu::visibility("hidden")]] uint8_t *get_key(void) {
uint8_t* mem = (uint8_t*)malloc(2);
if (mem == 0) { return 0; }
mem[0] = 0xFD;
mem[1] = 0xFE;
return mem;
};client.c$ cat client.c
#include <stdint.h>
#include "vis.h"
int main(void) {
int res = check_timestamp();
uint8_t* mem = get_key();
return res + mem[0];
}Bash commandsBuild the static archive file: The functions are GLOBAL HIDDEN as expected. Compile the client and statically link it with the out.bin: file format elf64-x86-64
Disassembly of section .text:
0000000000001060 <main>:
1060: f3 0f 1e fa endbr64
1064: 53 push %rbx
1065: e8 06 01 00 00 call 1170 <check_timestamp>
106a: 89 c3 mov %eax,%ebx
106c: e8 0f 01 00 00 call 1180 <get_key>
1071: 0f b6 00 movzbl (%rax),%eax
1074: 01 d8 add %ebx,%eax
1076: 5b pop %rbx
1077: c3 retBoth 0000000000001170 <check_timestamp>:
1170: f3 0f 1e fa endbr64
1174: b8 fe ff ff ff mov $0xfffffffe,%eax
1179: c3 ret
117a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
0000000000001180 <get_key>:
1180: f3 0f 1e fa endbr64
1184: 48 83 ec 08 sub $0x8,%rsp
1188: bf 02 00 00 00 mov $0x2,%edi
118d: e8 be fe ff ff call 1050 <malloc@plt>
1192: 48 85 c0 test %rax,%rax
1195: 74 08 je 119f <get_key+0x1f>
1197: ba fd fe ff ff mov $0xfffffefd,%edx
119c: 66 89 10 mov %dx,(%rax)
119f: 48 83 c4 08 add $0x8,%rsp
11a3: c3 ret |
|
yes. thank you for this verbose answer, and apologies for not being clear with my question. i don't care much about the static lib for testing/debugging. we can just directly link to the individual .o files from the test/ directory - i only link to the static lib out of laziness, if we replace this all-visible-syms static lib with one where the internals are eclipsed, then i have no issue with replacing the liboprf.a linkage in |
|
The reason that I didn't package it into a proper It looks like the current direction is to properly package a static archive file for release use, and fix our tests so that it doesn't try to link against the archive file (which is built for release purposes now) It should be a simple fix: Just add another rule like this in the main makefile: liboprf_release.$(STATICEXT): liboprf_merged_localized.o
$(AR) rcs $@ $^But what do we do with the |
nah, i'd just link the .o files directly instead of the .a. as said, i just used the .a because i was lazy. i shouldn't be. i should be exact and accurate. |
- Build all objects explicitly. The rules are generated by putting `eval` inside a `foreach` loop. Guides for writing dynamic rules are in the comments. - `$(info )` : The space is strictly required.
- Pass in `-DSODIUM_STATIC=1 -DSODIUM_EXPORT=""` to hide the libsodium symbols that we vendorized into liboprf - In the previous commit, `-fvisibility=hidden` was over-ruled by SODIUM_EXPORT - Ref: <https://libsodium.gitbook.io/doc/usage>
|
I packaged the merged-localized object into During installation, All translation units are now explicitly built. Source files in the master branch are built twice because Added terminal colors ( Finally, there is a way to hide the libsodium symbols without changing the source. The define flags are officially documented here. |
|
|
most of this makes sense to me, however ce5671d introduces an unnecessary layer of abstraction which gives me bad vibes. you say this is explicit. explicit for me would be: but that is bad for maintainability (see below for uninstall deps missing). i don't really see what is the benefit of this abstraction you proposed and if it is proportional to the extra cognitive load understanding the whole thing. afaics this is only for the case when we have to compile the extrasources with vis hidden, for that we can have one explicit rule: i would love to merge the rest though. i notice (my fault!) that the uninstall target does not uninstall i will add these when i merge (and i'll fix the refs to liboprf.a from the |
|
thank you for your patience with this. i had some tragic events that made me have to take a break from a lot of things. in the final version of the merge i did the changes i proposed above, like replace liboprf.a refs with explicit refs to ../XXX.c files in the tests/makefile, replacing the dynamic rules section with an explicit section for compiling the vendored sodium file, and i added the missing uninstall files. i think that more or less sums up my changes. thank you so very much for for this PR, these changes are much appreciated! |
2 participants
Remove
noise_xk/liboprf-noiseXK.sofrom the GCC input arguments as it is redundant.Add a "merged" object file and a "merged localized" object file. The former exposes all "hidden" functions and should be useful for testing. The latter marks all hidden functions "local hidden" which makes it suitable for static linking with client code.
liboprf_merged.o
liboprf_merged_localized.o