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

minlib tech with optimized rebuild #477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions cos
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ script=
name=
binary=
arch=

minlib=

initialize()
{
case ${arch} in
Expand Down Expand Up @@ -62,10 +63,16 @@ compose()
exit 1
fi

echo "[cos executing] src/composer/target/debug/compose $script $name"
src/composer/target/debug/compose $script $name
if [ -z "$minlib" ]; then
echo "[cos executing] src/composer/target/debug/compose $script $name"
src/composer/target/debug/compose $script $name
local dir="system_binaries/cos_build-${name}"
else
echo "[cos executing] src/composer/target/debug/compose $script $name $minlib"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to replicate the echo here. If you simply print it out before the conditional, if $minlib is nothing, it won't print out anything. Which should be identical in behavior to this code, without the replication. I might be wrong.

src/composer/target/debug/compose $script $name $minlib
local dir="system_binaries/cos_build_minlib-${name}"

local dir="system_binaries/cos_build-${name}"
fi

tools/build_iso.sh ${dir}/cos.img
}
Expand Down Expand Up @@ -157,6 +164,7 @@ case $1 in
compose )
script=$2
name=$3
minlib=$4
compose
;;
run )
Expand Down
7 changes: 7 additions & 0 deletions src/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ composer:
component:
$(MAKE) $(MAKEOPTIONS) -C components component

component_minlib:
$(MAKE) $(MAKEOPTIONS) -C components component_minlib

component_dir:
$(MAKE) $(MAKEOPTIONS) -C components component_dir


comps:
$(info )
$(info ***********************************************)
Expand Down
8 changes: 8 additions & 0 deletions src/components/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,11 @@ init:

component:
$(MAKE) $(MAKEOPTIONS) -C implementation component

component_minlib:
$(MAKE) $(MAKEOPTIONS) -C lib component_minlib
$(MAKE) $(MAKEOPTIONS) -C interface component_minlib

component_dir:
$(MAKE) $(MAKEOPTIONS) -C implementation component_dir

5 changes: 5 additions & 0 deletions src/components/implementation/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ all:
$(info ***********************************************)
$(info *********[ Building Implementations ]**********)
$(info ***********************************************)
$(info $(SUBDIRS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is likely just for debugging, I'm assuming. If so, it should not appear in a PR.

@for dir in $(SUBDIRS) ; do \
$(MAKE) $(MAKEOPTIONS) -C $$dir ; \
done
Expand All @@ -25,3 +26,7 @@ distclean:

component:
$(MAKE) $(MAKEOPTIONS) -C $(COMP_INTERFACE) component

component_dir:
$(MAKE) $(MAKEOPTIONS) -C $(COMP_INTERFACE) component_dir

5 changes: 5 additions & 0 deletions src/components/implementation/Makefile.subdir
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,9 @@ clean:
component:
$(MAKE) -C $(COMP_NAME) component


.PHONY: component_dir
component_dir:
$(MAKE) -C $(COMP_NAME) component_dir

-include $(SOURCE_DEPENDENCIES)
17 changes: 13 additions & 4 deletions src/components/implementation/Makefile.subsubdir
Original file line number Diff line number Diff line change
Expand Up @@ -79,28 +79,37 @@ COMP_DEPLIBDIRS_CLEAN=$(foreach D,$(COMP_DEPS_CLEAN),$(if $(wildcard $(INTERDIR)
COMP_EXPIF_OBJS=$(foreach I,$(COMP_INTERFACES_CLEAN),$(INTERDIR)/$(I)/cosrt_s_stub.o)
COMP_DEP_OBJS=$(foreach D,$(COMP_IFDEPS_CLEAN),$(INTERDIR)/$(D)/cosrt_c_stub.o)

DEP_DIRS := $(foreach obj,$(COMP_DEP_OBJS),$(dir $(obj)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a comment block above these to say what they are trying to do?

DEP_DIRS := $(foreach dir,$(DEP_DIRS),$(dir $(patsubst %/,%,$(dir))))
DEP_OBJS := $(foreach obj,$(DEPENDENCY_LIBOBJS),$(dir $(obj)))
LIB_DIRS := $(foreach lib,$(DEPENDENCY_LIBPATH),$(subst -L,,$(lib)))

# NOTE: we're currently ignoring the *variants* library requirements,
# which will break if an interface's code requires a library

comp_header:
$(info | Composing $(COMP_INTERFACE).$(COMP_NAME) for variable $(COMP_VARNAME) by linking with:)
$(info | Exported interfaces: $(COMP_INTERFACES_CLEAN))
$(info | Interface dependencies: $(COMP_IFDEPS_CLEAN))
$(info | Libraries: $(DEPENDENCY_LIBS) $(DEPENDENCY_LIBOBJS))
$(info | Libraries:$(DEPENDENCY_LIBS) $(DEPENDENCY_LIBOBJS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this was an accident, undo it.


.PHONY: component
component: clean comp_header $(COMPOBJ)
$(if $(COMP_INITARGS_FILE), $(CC) $(INCLUDE) $(CFLAGS) -c -o $(COMP_INITARGS_FILE:%.c=%.o) $(COMP_INITARGS_FILE))
$(if $(COMP_INITARGS_FILE), $(CC) $(INCLUDE) $(CFLAGS) $(CFLAGS_MINLIB) -c -o $(COMP_INITARGS_FILE:%.c=%.o) $(COMP_INITARGS_FILE))
$(if $(COMP_TAR_FILE), cp $(COMP_TAR_FILE) $(TAR_SYMBOL_NAME))
$(if $(COMP_TAR_FILE), $(LD) $(LDFLAGS) -r -b binary $(TAR_SYMBOL_NAME) -o $(COMP_TAR_FILE).o; rm $(TAR_SYMBOL_NAME))
$(LD) $(LDFLAGS) -r -o $(COMPNAME).linked_libs_ifs.o $(COMPOBJ) $(COMP_EXPIF_OBJS) $(COMP_DEP_OBJS) $(if $(COMP_INITARGS_FILE), $(COMP_INITARGS_FILE:%.c=%.o)) $(if $(COMP_TAR_FILE), $(COMP_TAR_FILE).o) $(COMP_DEPLIBDIRS_CLEAN) $(COMP_DEPLIBS_CLEAN) $(LIB_FLAGS)
$(MUSLCC) $(COMPNAME).linked_libs_ifs.o $(MUSLCFLAGS) $(LINKFLAG) -o $(COMPNAME).linked_musl.o
$(LD) $(LDFLAGS) -Ttext=$(COMP_BASEADDR) -T $(COMP_LD_SCRIPT) -o $(COMP_OUTPUT) $(COMPNAME).linked_musl.o
$(LD) $(LDFLAGS) $(LDFLAGS_MINLIB) -Ttext=$(COMP_BASEADDR) -T $(COMP_LD_SCRIPT) -o $(COMP_OUTPUT) $(COMPNAME).linked_musl.o

.PHONY: component_dir
component_dir:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This rule isn't really related to the _dir as a primary abstraction. They are meant to simply output dependencies. So why not compoent_dependecies? Note this change would require changes in many makefiles as they require consistent rule names.

$(info $(LIB_DIRS) $(DEP_DIRS) $(DEP_OBJS))

# NOTE: make CFLAGS/CXXFLAGS/ASFLAGS argument first because it includes kernel headers/musl headers first
%.o:%.c
$(info | [CC] $<: Compiling)
@$(CC) $(CFLAGS) $(INCLUDE) -o $@ -c $<
@$(CC) $(CFLAGS) $(CFLAGS_MINLIB) $(INCLUDE) -o $@ -c $<

%.o:%.cc
$(info | [CXX] $<: Compiling)
Expand Down
2 changes: 1 addition & 1 deletion src/components/implementation/comp_x86_64.ld
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ SECTIONS
. = SIZEOF_HEADERS;

/* start the text/read-only sections */
.text : ALIGN(4096) { *(.text*) } :text
.text : ALIGN(4096) { KEEP(*(.text.__cosrt_c_cosrtdefault)) *(.text*) } :text
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the spacing on this. Feel free to use multiple lines if that works with the syntax here.

.text : ALIGN(4096) {
    KEEP(*(.text.__cosrt_c_cosrtdefault))
    *(.text*) 
} :text

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern with this is this: if this is necessary to keep symbols around, don't we need an automated way to construct all of the symbols, not just the default entry point?

.rodata : { *(.rodata*) }
.eh_frame : { *(.eh_frame*) }

Expand Down
2 changes: 2 additions & 0 deletions src/components/implementation/pong/pingpong/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,6 @@ LIBRARY_DEPENDENCIES = component
# the build to fail. The build system does not validate this
# minimality; that's on you!



include Makefile.subsubdir
13 changes: 13 additions & 0 deletions src/components/interface/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ all:
$(info ***********************************************)
$(info ************[ Building interfaces ]************)
$(info ***********************************************)
$(info $(INTERFACES))
@for dir in $(INTERFACES) ; do \
$(MAKE) $(MAKEOPTIONS) -C $$dir ; \
done
Expand All @@ -24,3 +25,15 @@ cp:

.PHONY: init
init: clean

INTERFACE_DIRS := $(shell echo "$(MINLIB_DIRS)" | tr ' ' '\n' | grep '//interface//')
.PHONY: component_minlib
component_minlib:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is an interface, not a component, we want a better name for all of this.

Naming is hard!

$(info ***********************************************)
$(info ********[ ReBuilding MinLib Interface]*********)
$(info ***********************************************)
$(info $(INTERFACE_DIRS))
@for dir in $(INTERFACE_DIRS) ; do \
$(MAKE) $(MAKEOPTIONS) -C $$dir clean ; \
$(MAKE) $(MAKEOPTIONS) -C $$dir ; \
done
3 changes: 2 additions & 1 deletion src/components/interface/Makefile.subsubdir
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,11 @@ print:

%.o:%.c
$(info | [CC] Compiling c file $< into $@)
@$(CC) $(CFLAGS) $(DEP_INC) -c -o $(@) $<
@$(CC) $(CFLAGS) $(CFLAGS_MINLIB) $(DEP_INC) -c -o $(@) $<

$(S_SSTUB_OBJ):$(SSTUB_FILE)
$(info | [AS] Creating server asm stubs for $(IFNAME))
$(info | include paths are $(DEP_INC))
@$(AS) -DCOS_SERVER_STUBS $(ASFLAGS) $(DEP_INC) -c -o $@ $^

$(C_UCAP_STUB_OBJ):$(SSTUB_FILE)
Expand Down
16 changes: 16 additions & 0 deletions src/components/lib/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ all:
$(info **************************************************)
$(info **************[ Building Libraries ]**************)
$(info **************************************************)
$(info $(SUBDIRS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove debugging print outs.

@for dir in $(SUBDIRS) ; do \
echo "*************************[TASK START]*************************";\
echo " [ Building $$dir ]";\
Expand Down Expand Up @@ -125,3 +126,18 @@ distclean:
echo "*************************[TASK STOP]*************************";\
echo ""; \
done

LIB_DIRS := $(shell echo "$(MINLIB_DIRS)" | tr ' ' '\n' | grep '/lib/')
component_minlib:
Copy link
Collaborator

Choose a reason for hiding this comment

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

See other comment about this name.

$(info **************************************************)
$(info ***********[ ReBuilding MinLib Library]***********)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be Rebuilding, I think.

$(info **************************************************)
$(info $(LIB_DIRS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar, remove debugging printouts.

@for dir in $(LIB_DIRS) ; do \
echo "*************************[TASK START]*************************";\
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't like all of this formatting, but since you're just emulating something that was previously in the repo, thank you for being consistent!

echo " [ Building $$dir ]";\
$(MAKE) PLATFORM=$(PLATFORM) $(MAKEOPTIONS) -C $$dir clean || exit $$?; \
$(MAKE) PLATFORM=$(PLATFORM) $(MAKEOPTIONS) -C $$dir || exit $$?; \
echo "*************************[TASK STOP]*************************";\
echo ""; \
done
2 changes: 1 addition & 1 deletion src/components/lib/Makefile.lib
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ $(SHLIB_FILE): $(OBJS)

%.o:%.c
$(info | [CC] Compiling C file $< into $@)
@$(CC) $(CFLAGS) $(CINC) $(DEPENDENCY_INCPATH) -c -o $@ $<
@$(CC) $(CFLAGS) $(CFLAGS_MINLIB) $(CINC) $(DEPENDENCY_INCPATH) -c -o $@ $<

# redirect error output to /dev/null so that it will not display
# errors when cleaning, this does not affect gcc's error output when
Expand Down
70 changes: 63 additions & 7 deletions src/composer/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ fn comp_gen_make_cmd(
tar_file: &Option<String>,
id: &ComponentId,
s: &SystemState,
is_minlib: bool,
) -> String {
let c = component(&s, id);
let ds = deps(&s, id);
Expand Down Expand Up @@ -295,14 +296,31 @@ fn comp_gen_make_cmd(
let compid = s.get_named().rmap().get(&c.name).unwrap();
let baseaddr = s.get_address_assignments().component_baseaddr(compid);

let cmd = format!(
let cmd = if is_minlib {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets improve the existing code (before your changes) a little bit!

We can remove the let cmd, and instead if the if is_minlib {... blocks are the last things in the file, what they return, the function will return! So we don't even need the cmd variable.

format!(
r#"make --quiet -C src COMP_INTERFACES="{}" COMP_IFDEPS="{}" COMP_LIBDEPS="" COMP_INTERFACE={} COMP_NAME={} component_dir"#,
if_exp, if_deps, &decomp[0], &decomp[1]
)
}
else {format!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting is wrong here. You have to run rustfmt so that it will automatically format it in the conventional way.

r#"make -C src COMP_INTERFACES="{}" COMP_IFDEPS="{}" COMP_LIBDEPS="" COMP_INTERFACE={} COMP_NAME={} COMP_VARNAME={} COMP_OUTPUT={} COMP_BASEADDR={:#X} {} component"#,
if_exp, if_deps, &decomp[0], &decomp[1], &c.name, output_name, baseaddr, &optional_cmds
);
)
};

cmd
}

fn comp_gen_minlib_make_cmd(
minlib_dirs: &String,
cflags_minlib: &String
) -> String {
format!(
r#"make -C src MINLIB_DIRS="{}" CFLAGS_MINLIB="{}" component_minlib"#,
minlib_dirs, cflags_minlib
)
}

fn kern_gen_make_cmd(input_constructor: &String, kern_output: &String, _s: &SystemState) -> String {
format!(
r#"make -C src KERNEL_OUTPUT="{}" CONSTRUCTOR_COMP="{}" plat"#,
Expand All @@ -312,12 +330,14 @@ fn kern_gen_make_cmd(input_constructor: &String, kern_output: &String, _s: &Syst

pub struct DefaultBuilder {
builddir: String,
minlib: bool,
}

impl DefaultBuilder {
pub fn new() -> Self {
DefaultBuilder {
builddir: "/dev/null".to_string(), // must initialize, so error out if you don't
minlib: false,
}
}
}
Expand All @@ -330,15 +350,21 @@ fn compdir_check_build(comp_dir: &String) -> Result<(), String> {

Ok(())
}
use std::process::Command;

impl BuildState for DefaultBuilder {
fn initialize(&mut self, name: &String, _s: &SystemState) -> Result<(), String> {
fn initialize(&mut self, name: &String, _s: &SystemState, minlib: bool) -> Result<(), String> {
let pwd = env::current_dir().unwrap();
let dir = format!("{}/system_binaries/cos_build-{}", pwd.display(), name);

let dir = if minlib {
format!("{}/system_binaries/cos_build-{}-minlib", pwd.display(), name)
} else {
format!("{}/system_binaries/cos_build-{}", pwd.display(), name)
};

reset_dir(&dir)?;
self.builddir = dir;
Copy link
Collaborator

Choose a reason for hiding this comment

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

self = DefaultBuilder {
    builddir: dir,
    minlib,
};

Is better here.


self.minlib = minlib;
Ok(())
}

Expand Down Expand Up @@ -378,7 +404,37 @@ impl BuildState for DefaultBuilder {
let p = state.get_param_id(&id);
let output_path = self.comp_obj_path(&id, &state)?;

let cmd = comp_gen_make_cmd(&output_path, p.param_prog(), p.param_fs(), &id, &state);
if self.minlib {
//get the related sub directories
let subdirs_cmd = comp_gen_make_cmd(&output_path, p.param_prog(), p.param_fs(), &id, &state, true);
let (out1, err1) = exec_pipeline(vec![subdirs_cmd.clone()]);

if !err1.is_empty() {
return Err(format!("Error in executing pipeline for component_dir: {}", err1));
}

let mut cflag_minlib = "-ffunction-sections -fdata-sections";

//rebuild the files under the related sub directories
let output = Command::new("make")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be able to use exec_pipeline here instead of pulling in the dependency on Command, right? I think you explained why you couldn't, but I can't figure out now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coming back to this now. I'm guessing you need the actual output, and exec_pipeline doesn't provide that. I think that the correct abstraction level is to provide an exec_pipeline variant that returns (stdout, stderr) output strings.

.arg("-C")
.arg("src")
.arg(format!("MINLIB_DIRS={}", out1))
.arg(format!("CFLAGS_MINLIB={}", cflag_minlib))
.arg("component_minlib")
.output()
.expect("Failed to execute make command");

if output.status.success() {
let stdout = String::from_utf8_lossy(&output.stdout);
println!("Output: {}", stdout);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These seem like they are focused on debugging output? not sure if we really want this output normally.

} else {
let stderr = String::from_utf8_lossy(&output.stderr);
eprintln!("Error: {}", stderr);
}
}

let cmd = comp_gen_make_cmd(&output_path, p.param_prog(), p.param_fs(), &id, &state, false);

let name = state.get_named().ids().get(id).unwrap();
println!(
Expand Down Expand Up @@ -413,7 +469,7 @@ impl BuildState for DefaultBuilder {
let binary = self.comp_obj_path(&c, &s)?;
let argsfile = constructor_serialize_args(&c, &s, self)?;
let tarfile = constructor_tarball_create(&c, &s, self)?;
let cmd = comp_gen_make_cmd(&binary, &argsfile, &tarfile, &c, &s);
let cmd = comp_gen_make_cmd(&binary, &argsfile, &tarfile, &c, &s, false);

let name = s.get_named().ids().get(c).unwrap();
println!(
Expand Down
7 changes: 5 additions & 2 deletions src/composer/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,20 @@ pub fn exec() -> Result<(), String> {

let arg1 = args.next();
let arg2 = args.next();
let arg3 = args.next(); // Optional third argument

if None == arg1 || None == arg2 {
return Err(format!(
"usage: {} <sysspec>.toml <buildname>",
"usage: {} <sysspec>.toml <buildname> [minlib]",
program_name.unwrap()
));
}

let b_minlib= arg3.as_ref().map(|s| s == "minlib").unwrap_or(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what the b_ stands for here. Maybe use_minlib?


let mut sys = SystemState::new(arg1.unwrap());
let mut build = DefaultBuilder::new();
build.initialize(&arg2.unwrap(), &sys)?;
build.initialize(&arg2.unwrap(), &sys, b_minlib)?;

sys.add_parsed(SystemSpec::transition(&sys, &mut build)?);
sys.add_named(CompTotOrd::transition(&sys, &mut build)?);
Expand Down
2 changes: 1 addition & 1 deletion src/composer/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl SystemState {
// that file already exists. Use unique names for files, and use the
// component-namespacing of names for per-component files.
pub trait BuildState {
fn initialize(&mut self, name: &String, s: &SystemState) -> Result<(), String>; // must be called *before* the following functions
fn initialize(&mut self, name: &String, s: &SystemState, minlib: bool) -> Result<(), String>; // must be called *before* the following functions
fn file_path(&self, file: &String) -> Result<String, String>; // create a path in the build directory for a file
fn comp_dir_path(&self, c: &ComponentId, state: &SystemState) -> Result<String, String>; // the component's object
fn comp_file_path(
Expand Down
2 changes: 1 addition & 1 deletion src/composer/src/resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ enum CapRes {
Comp(ComponentId),
}

const BOOT_CAPTBL_FREE: u32 = 60;
const BOOT_CAPTBL_FREE: u32 = 52;

// The equivalent of the C __captbl_cap2sz(c)
fn cap_sz(cap: &CapRes) -> u32 {
Expand Down
Loading