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

extractor: Make extraction errors fatal #37

Merged

Conversation

woodruffw
Copy link
Collaborator

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.

@ianamason
Copy link
Member

@woodruffw seems like a reasonable improvement, but the CI is still complaining about shadowing.
Are you gonna fix that or should I?

@woodruffw
Copy link
Collaborator Author

woodruffw commented Aug 3, 2020 via email

@ianamason
Copy link
Member

I can wait. There is no rush. How is the other issue coming along? Weren't you guys working on issue #34 ?

@woodruffw
Copy link
Collaborator Author

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.

@woodruffw
Copy link
Collaborator Author

Fixed the shadowing warnings; will try to debug this:

cd libtecla; get-bc enhance; mv enhance.bc ../
mv: cannot stat ‘enhance.bc’: No such file or directory
make: *** [enhance.bc] Error 1
The command "./.travis/test.sh" exited with 2.

Booleans are default-initialized to false, so we only need
to assign to true in the success case.
@woodruffw woodruffw changed the title [WIP] extractor: Make extraction errors fatal extractor: Make extraction errors fatal Aug 4, 2020
@woodruffw woodruffw marked this pull request as ready for review August 4, 2020 14:51
@woodruffw
Copy link
Collaborator Author

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:

get-bc -b ./lib/libc.a
ERROR:Error reading the .llvm_bc section of ELF file clone.o.

...but this is to be expected, since clone.o comes from an assembly translation unit for which there can be no bitcode:

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 gllvm fail when the extracted/combined bitcode can't accurately reflect the original build?

@ianamason
Copy link
Member

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
still forge ahead, letting the user know about the missing stuff.

"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?

@woodruffw
Copy link
Collaborator Author

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).

@ianamason
Copy link
Member

Yes it makes sense in the semi automated setting. Our other design principle is to be a drop in replacement for wllvm.
Warts and all.

The more permissive current behavior remains the default.
@ianamason
Copy link
Member

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: iam@csl.sri.com

@ianamason ianamason merged commit 165d336 into SRI-CSL:master Aug 4, 2020
@woodruffw woodruffw deleted the ww/make-extraction-errors-fatal branch August 4, 2020 17:20
@woodruffw
Copy link
Collaborator Author

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?

@ianamason
Copy link
Member

Sure I will make a release this morning.

@ianamason
Copy link
Member

v1.2.7 is live.

@woodruffw
Copy link
Collaborator Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants