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
examples/linux64/hello-make: Example using single .c file and Makefile #39
examples/linux64/hello-make: Example using single .c file and Makefile #39
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
=======================================
Coverage 79.24% 79.24%
=======================================
Files 326 326
Lines 42496 42496
=======================================
Hits 33674 33674
Misses 8822 8822 Continue to review full report at Codecov.
|
As the description says, I don't think this should be merged as is, rather, this should be an example and motivation to fix the issues spotted and submitted. Currently, this is "blocked" on: #36 and #38 . The samples works around both issues, but that's the point: we should not give an example how to work around the issues, they should not be there in the first place. |
Nice job! I agree that the issues should be fixed before merging this example. Good example btw, and thanks for taking the time to test drive this project. Btw, you are probably one of the first test users, but I guess you already got that. Thanks for taking the time and reporting your experiences. |
examples/linux64/hello-make/hello.c
Outdated
// precluding from running as entrypoint). | ||
int _startup() | ||
{ | ||
asm("call main"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I recently added multiline asm syntax support if you want to use it:
asm(
"call main\n"
"jmp exit\n"
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I recently added multiline asm syntax
Well, I, multiline inline asm syntax wit those explicit \n's isn't particularly pretty, so given a choice, I used per-line asm's. But given that register info applies to the entire asm block, there's no choice, so will reformat as you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I mean, here there's no unavoidable need to reformat, but for consistency...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I, multiline inline asm syntax wit those explicit \n's isn't particularly pretty
Fully agree, this is ugly, but it is an artifact of string literals being glued together at the lexer level in C, so that's probably why it is required.
examples/linux64/hello-make/hello.c
Outdated
|
||
void syscall(int nr, int a, int b, int c) | ||
{ | ||
asm("mov rax, rdi"); // abi param 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added input parameters and clobber info to assembly code, so this should work as well:
asm(
"mov rax, %0 \n"
"mov rdi, %1 \n"
"mov rsi, %2 \n"
"mov rdx, %3 \n"
"syscall \n"
:
: "r" (nr), "r" (a), "r" (b), "r" (c) // input registers, patched into %0 .. %3
: "rax", "rdi", "rsi", "rdx" // clobber registers, handled by the register allocator
);
In this example, it might not make much sense, since we target linux anyways, but the code now makes some assumptions about the calling convention of the syscall function itself.
Off course, you might want to cross check the result of this with tools/ppci_explorer.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this example, it might not make much sense, since we target linux anyways, but the code now makes some assumptions about the calling convention of the syscall function itself.
Yeah, I guess where it comes into play is with optimizations and surrounding code. But it's definitely best practice to use it, and we want to show just that as example, so applying your suggested code verbatim.
hello.all.o: hello.o | ||
python -m ppci ld --layout linux64.ld $^ -o $@ | ||
|
||
# We build object files as usual with $(CC) (using the builtin make rule). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool that the builtin rule "just works" (tm)!
Sure, I'm very well aware what happens when you give something you dearly worked on for some time into hands of other people - they start to bend and abuse it in unpredictable ways, and all hell breaks loose ;-). In my case, I also oftentimes work on minimalist projects, which are supposed to do only what they're supposed to do. That's why I go thru the trouble of discussing things I see missing in PPCI and would like to see added/changed. Thanks for other comments - will act on them later. |
a152193
to
ca742e5
Compare
Updated inline asm as was suggested in comments, and also removed explicit objcopy step, per #32 (comment). Now, this looks much cleaner and solid! |
@@ -0,0 +1,15 @@ | |||
CC = python -m ppci cc | |||
CFLAGS = -O1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@windelbouwman: FYI, ppci-cc currently doesn't accept -O without arg, but it should (equivalent to -O1 per gcc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, indeed, this is a impedance mismatch with gcc. The way I implemented now is with argparse and accepting a series of optimization levels. The other option is to make each optimization target a seperate command line argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I implemented now is with argparse and accepting a series of optimization levels.
Yeah, I see the problem. argparse allows to specify an argument to short option either without a space or not, i.e. ppci-cc -O1
and ppci-cc -O 1
are equivalent, and -O
alone is not possible, as argparse expects an argument. gcc "solves" that by now allow a space:
$ gcc -c -O 1 hello.c
gcc: error: 1: No such file or directory
The other option is to make each optimization target a seperate command line argument.
That would make things like -Os
, -O1
, etc. essentially long options, and I thought argparse hardcodes long options to have --
prefix. But trying:
+compile_parser.add_argument(
+ "-Ofoo", help="optimize code", action="store_true"
+)
And running:
`ppci-cc -Ofoo`
doesn't lead to exceptions or visible artifacts. Oh, it's Python, so checking that it actually works is a matter of seconds:
Namespace(E=False, I=[], M=False, O='0', Ofoo=True, S=False, ast=False, c=True, define=[], freestanding=False, g=False, html_report=None, include=[], instrument_functions=False, ir=False, log=20, machine=None, mtune=[], output='hello.o', pycode=False, report=None, sources=[<_io.TextIOWrapper name='hello.c' mode='r' encoding='UTF-8'>], std='c99', super_verbose=False, text_report=None, trigraphs=False, undefine=[], verbose=0, wasm=False)
So, yep, seems to work, and hopefully it means we'll be able to achieve as close option compatibility with gcc as possible without switching to custom arg parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I suggest to fix this later, we'll figure a way :)
examples/linux64/hello-make/hello.c
Outdated
@@ -0,0 +1,58 @@ | |||
// Currently, it's not possible to specify executable entrypoint to ppci | |||
// linker - so far, the entrypoint is at the beginning of the code segment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the option to specify the entry point in the linker script by using:
ENTRY(startup)
Please refer to commit c078ba4 for the change.
Thanks for this improvement hint! I'm pretty happy with this change!
ca742e5
to
017ee66
Compare
Ok, with resolution of #36, I updated this sample to be like I initially wanted it to be: when inline asm is only used to implement syscall(), and main() is the entrypoint. From my PoV, this is now ready to merge. |
@@ -0,0 +1,9 @@ | |||
MEMORY code LOCATION=0x40000 SIZE=0x10000 { | |||
SECTION(reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reset
section can be removed. Note that you may want to add the ENTRY to main as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reset section can be removed.
Ack. Was originally copied verbatim from the "hello" sample, but makes sense to remove any unneeded stuff to not confuse readers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed, I'm removing it from the other sample code as well, to reduce clutter. The reset section was needed before the entry point option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you may want to add the ENTRY to main as well.
I pass it on the command line instead. The whole idea of this example is to show how to do things from command-line interface.
For as long as this "memory map layout" files aren't compatible with GNU ld "linker scripts", I'd personally prefer to avoid using them too much ;-). (I know that "ENTRY(sym)" syntax is exactly compatiable with GNU ld, thanks for that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, I missed that, thanks!
# Now link all compiled objects (here, just one) into executable. For this, | ||
# we need to provide linker with memory layout file. We'll also use main() | ||
# as the executable entrypoint. | ||
hello: hello.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to put the linux64.ld
file as dependency as well (since it is a dependency), but that did not work due to the nature of the $^
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to put the linux64.ld file as dependency as well (since it is a dependency), but that did not work due to the nature of the $^ variable.
Yep. You either need to get rid of special vars, using "materialized" file list, or so-called order-only dependencies might help, but few people know about them, so using them might be confusing. This is again to show simple, clear, concise usage.
And a linker script. Made possible by the inline assembly support, added recently. The sample still has to perform various workarounds of PPCI warts, which rather be fixed before merging this sample.
017ee66
to
629ff58
Compare
Pushed update with:
(yanked that ALIGN too, as it's already aligned, let's show people the simplest possible usage). |
And a linker script. Made possible by the inline assembly support, added
recently.
The sample still has to perform various workarounds of PPCI warts, which
rather be fixed before merging this sample.