-
Notifications
You must be signed in to change notification settings - Fork 34
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
extractor: Make extraction errors fatal #37
extractor: Make extraction errors fatal #37
Conversation
@woodruffw seems like a reasonable improvement, but the CI is still complaining about shadowing. |
I can fix it tomorrow ET! Feel free to take over if you’d like it merged before then, though.
Sent from mobile. Please excuse my brevity.
… On Aug 3, 2020, at 6:57 PM, Ian A Mason ***@***.***> wrote:
@woodruffw seems like a reasonable improvement, but the CI is still complaining about shadowing.
Are you gonna fix that or should I?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I can wait. There is no rush. How is the other issue coming along? Weren't you guys working on issue #34 ? |
Yep, we were. We ended up taking another approach for capturing the command-line flags, so we haven't made a ton of progress on building it into gllvm itself. It's something that I can take another look at when I have some free time, though. |
Fixed the shadowing warnings; will try to debug this:
|
Booleans are default-initialized to false, so we only need to assign to true in the success case.
Alright, this should be good for review now (apologies for the churn, I'm not much of a Go programmer...). The CI is failing with this error:
...but this is to be expected, since gclang -std=c99 -nostdinc -ffreestanding -Wa,--noexecstack -D_XOPEN_SOURCE=700 -I./arch/LLVM -I./arch/generic -Iobj/src/internal -I./src/include -I./src/internal -Iobj/include -I./include -Os -pipe -fomit-frame-pointer -fno-unwind-tables -fno-asynchronous-unwind-tables -ffunction-sections -fdata-sections -Werror=implicit-function-declaration -Werror=implicit-int -Werror=pointer-sign -Werror=pointer-arith -Qunused-arguments -c -o obj/src/thread/LLVM/clone.o src/thread/LLVM/clone.s So this is more of a design question: should |
I think this is why we just warn when a segment is not found, because there is not much we can do, but we can "Don't let perfect be the enemy of good" is the design principle I think. Now your version will no longer produce a final (less than perfect) bitcode result? Maybe this strictness would better be enabled by a command line switch? The default would then be the relaxed mode? What is your motivation for this "strict" mode? |
Yep, exactly. It currently bails out instead of producing an (incomplete) unified bitcode module. Enabling this via a command-line switch sounds good to me. I can do that in a bit. My motivation is a static analysis ecosystem that does semi-automated builds of target programs. This strict mode makes it easier to catch eventual problems with (re)compiling the bitcode due to missing symbols (i.e. the ones present in assembly TUs). |
Yes it makes sense in the semi automated setting. Our other design principle is to be a drop in replacement for wllvm. |
The more permissive current behavior remains the default.
Thanks. If you can I would like to hear more about the "We ended up taking another approach for capturing the command-line flags" saga. Doesn't have to be here, could be there: [email protected] |
Thanks for the merge! I'll see what I can share about the command-line flag capturing. Would it be possible to get a quick release cut for these changes? |
Sure I will make a release this morning. |
v1.2.7 is live. |
Thanks! |
Prior to this, running
get-bc
on a binary with no.llvm_bc
(or corresponding Mach-O section) would print an error message but exit with 0.This turns those errors into true failures, making it easier to diagnose when
get-bc
has been run on an incorrect or mis-generated binary.In progress.