Page MenuHomePhabricator

llvm-nm: add flag to suppress no symbols warning
ClosedPublic

Authored by keith on Jul 3 2020, 9:06 PM.

Diff Detail

Event Timeline

keith created this revision.Jul 3 2020, 9:06 PM
Herald added a project: Restricted Project. · View Herald Transcript
jhenderson added a subscriber: grimar.

Added a couple more people with experience with Darwin, to see if they have any thoughts on the option name/whether it would be helpful to be identical etc. Also added @grimar/@MaskRay/@rupprecht who have a lot of experience with the binutils too.

Please update the llvm-nm documentation found in the CommandGuide directory to include the new switch as part of this change.

I'm happy to support adding the option, if there's a desire for it. It's trivial enough that it seems reasonable. I don't know about the name of it, and would prefer a shorter name if anybody can come up with one (my only suggestion is --no-no-symbols-warning which is almost as verbose and possibly more confusing), but if matching the libtool name is useful, I'm not actively opposed to it.

llvm/test/tools/llvm-nm/X86/nm-no-symbols.test
14

If I'm right, llvm-nm should not print any output at all in this case? If that's the case, I'd change this line to: # NO-WARNING-NOT: {{.}} to make it a little more robust against possible further changes/typos etc.

grimar added inline comments.Jul 6 2020, 2:24 AM
llvm/test/tools/llvm-nm/X86/nm-no-symbols.test
13

-no-warning-for-no-symbols -> --no-warning-for-no-symbols

(there is an agreement to use double dash for long versions of options in tests.
E.g: use --reverse-sort instead of -reverse-sort or just use -r.
I think one of the reasons was to make the difference between things like --reverse-sort and -r everse-sort more obvious).

I cannot find any search result about no-warning-for-no-symbols. Is -no-warning-for-no-symbols really an existing option? libtool is an ar like tool.

Second, I wonder how you are going to plug -no-warning-for-no-symbols into a build system. If you only parse stdout, you can ignore stderr. Even if you do, you can probably use grep -v '^no symbols'. This will have better portability (supported on older nm, supported on other binary formats).

I cannot find any search result about no-warning-for-no-symbols. Is -no-warning-for-no-symbols really an existing option? libtool is an ar like tool.

I found it by looking for underscores instead of hyphens: -no_warning_for_no_symbols.
However, the flag is an ar/ranlib/libtool flag, not nm, AFAICT.

Second, I wonder how you are going to plug -no-warning-for-no-symbols into a build system. If you only parse stdout, you can ignore stderr. Even if you do, you can probably use grep -v '^no symbols'. This will have better portability (supported on older nm, supported on other binary formats).

I agree this is likely the simpler option (just add 2> /dev/null to the build script using nm)

keith added a comment.Jul 6 2020, 3:08 PM

I cannot find any search result about no-warning-for-no-symbols. Is -no-warning-for-no-symbols really an existing option? libtool is an ar like tool.

I found it by looking for underscores instead of hyphens: -no_warning_for_no_symbols.
However, the flag is an ar/ranlib/libtool flag, not nm, AFAICT.

Yea sorry I should have been more clear, it's not the _exact_ same spelling because of the conventions used in nm with - instead of _.

Second, I wonder how you are going to plug -no-warning-for-no-symbols into a build system. If you only parse stdout, you can ignore stderr. Even if you do, you can probably use grep -v '^no symbols'. This will have better portability (supported on older nm, supported on other binary formats).

I agree this is likely the simpler option (just add 2> /dev/null to the build script using nm)

If folks feel strongly about this that would definitely work, this felt like a safer way to silence this for the future for me, but if you all think it's not worth adding an option for that's fine.

I cannot find any search result about no-warning-for-no-symbols. Is -no-warning-for-no-symbols really an existing option? libtool is an ar like tool.

I found it by looking for underscores instead of hyphens: -no_warning_for_no_symbols.
However, the flag is an ar/ranlib/libtool flag, not nm, AFAICT.

Yea sorry I should have been more clear, it's not the _exact_ same spelling because of the conventions used in nm with - instead of _.

Second, I wonder how you are going to plug -no-warning-for-no-symbols into a build system. If you only parse stdout, you can ignore stderr. Even if you do, you can probably use grep -v '^no symbols'. This will have better portability (supported on older nm, supported on other binary formats).

I agree this is likely the simpler option (just add 2> /dev/null to the build script using nm)

If folks feel strongly about this that would definitely work, this felt like a safer way to silence this for the future for me, but if you all think it's not worth adding an option for that's fine.

I don't have strong opinions either way. I think there probably is some benefit to adding the new option: one counter-point to redirecting stderr to /dev/null is that will hide any real error messages llvm-nm (e.g. caused by a missing input file). I don't think that's a good idea personally. That being said, if stderr is being redirected somewhere other than /dev/null maybe it's okay. Another issue is that because stderr and stdout are often handled independently, but end up in the same final output, you can end up with the "no symbols" message appearing quite some way from where it really belongs, which could be slightly confusing. However, it's not an issue I actually have at the moment.

Another possible benefit (again not something I personally have worried about, but one I thought of) is that users could specify the option to avoid having to special case parsing scripts to handle no symbols. A naive script would typically just iterate through the lines and expect each one to correspond to a symbol, so if there are none, the parser might break.

Another option name could just be --quiet, although there's a small risk that could clash in the future with a GNU option.

I cannot find any search result about no-warning-for-no-symbols. Is -no-warning-for-no-symbols really an existing option? libtool is an ar like tool.

I found it by looking for underscores instead of hyphens: -no_warning_for_no_symbols.
However, the flag is an ar/ranlib/libtool flag, not nm, AFAICT.

Yea sorry I should have been more clear, it's not the _exact_ same spelling because of the conventions used in nm with - instead of _.

Second, I wonder how you are going to plug -no-warning-for-no-symbols into a build system. If you only parse stdout, you can ignore stderr. Even if you do, you can probably use grep -v '^no symbols'. This will have better portability (supported on older nm, supported on other binary formats).

I agree this is likely the simpler option (just add 2> /dev/null to the build script using nm)

If folks feel strongly about this that would definitely work, this felt like a safer way to silence this for the future for me, but if you all think it's not worth adding an option for that's fine.

I don't have strong opinions either way. I think there probably is some benefit to adding the new option: one counter-point to redirecting stderr to /dev/null is that will hide any real error messages llvm-nm (e.g. caused by a missing input file). I don't think that's a good idea personally.

The return code would be 1 for a missing input file, which should fail whatever build step/etc. is executing llvm-nm. If you're ignoring that return code, you have other problems :)

That said, I'm on the fence here -- it doesn't seem all that complex to add it, but it also doesn't seem worth the extra (minor) complexity. Maybe I don't understand the use case well enough. Is there a build step executing llvm-nm that chokes on this? Can you say more about the use case?

That being said, if stderr is being redirected somewhere other than /dev/null maybe it's okay. Another issue is that because stderr and stdout are often handled independently, but end up in the same final output, you can end up with the "no symbols" message appearing quite some way from where it really belongs, which could be slightly confusing. However, it's not an issue I actually have at the moment.

Another possible benefit (again not something I personally have worried about, but one I thought of) is that users could specify the option to avoid having to special case parsing scripts to handle no symbols. A naive script would typically just iterate through the lines and expect each one to correspond to a symbol, so if there are none, the parser might break.

Since "no symbols" is printed to stderr, not stdout, you would have to go out of your way to have such a script need to handle "no symbols" as input by joining stdout and stderr. Normal pipelining ($ llvm-nm foo.o | script) would drop stderr.

Another option name could just be --quiet, although there's a small risk that could clash in the future with a GNU option.

keith added a comment.Jul 16 2020, 1:41 PM

Is there a build step executing llvm-nm that chokes on this? Can you say more about the use case?

Our use case for this is just scripts that analyze the symbols from some binaries, separately from our build system. Theoretically we could drop stderr (since we do handle the error code) and that might be "good enough". I would be a _bit_ worried without reading the rest of the code here whether by dropping stderr would lose any other "more-important" warnings (I might also be worried about this if we had a more generic --quiet option). I see this warning as pretty noisy for large static library targets where having object files with no symbols based on your deployment target is often expected in the libraries we're using.

MaskRay requested changes to this revision.Feb 8 2021, 8:26 PM

(So that it can leave people's review queue)

This revision now requires changes to proceed.Feb 8 2021, 8:26 PM
keith updated this revision to Diff 322403.Feb 9 2021, 8:39 AM

Update style in test

keith marked 2 inline comments as done.Feb 9 2021, 8:39 AM
MaskRay requested changes to this revision.Feb 9 2021, 9:46 AM

Sorry, I keep thinking this is not worthwhile because the advantage does not justify a new option no-warning-for-no-symbols. If you think it is justifiable, coordination on binutils@sourceware.org and making them accept an option as well is way to proceed here.

This revision now requires changes to proceed.Feb 9 2021, 9:46 AM
keith abandoned this revision.Feb 9 2021, 9:48 AM

thanks for the review

@MaskRay I haven't heard of coordination with binutils, can you say more about that? I'm particularly curious because llvm-nm, and other llvm tools, have many options not present in binutils. As an example @keith added --no-weak previously.

How do flags get justified? I would accept this, because the complexity is so low (a bool used in one place), and non-breaking. But who am I to decide :)

MaskRay added a comment.EditedFeb 9 2021, 8:01 PM

@MaskRay I haven't heard of coordination with binutils, can you say more about that? I'm particularly curious because llvm-nm, and other llvm tools, have many options not present in binutils.

In recent years I think we are more careful about documentation/tests and require justification for options.

As an example @keith added --no-weak previously.

The name --no-weak is fine. What I'd ask is the rationale for the new option: why is there a need to list non-weak symbols?
A potential risk with D48751 is that if GNU nm uses -W for other purposes, we may have a trouble.

Another example: llvm-nm -s has been taken for segment section. In nm, it means --print-armap.

How do flags get justified? I would accept this, because the complexity is so low (a bool used in one place), and non-breaking. But who am I to decide :)

I think it is case by case. For this one, 2> /dev/null is a simple and easy workaround. I find it difficult to argue against this choice. There was an equally good suggestion --quiet we cannot refute.

When there is doubt, it seems a good idea if the author is willing to engage with the other implementation. The adoption can serve as a tiebreak.

keith added a comment.Feb 9 2021, 8:20 PM

I think it is case by case. For this one, 2> /dev/null is a simple and easy workaround. I find it difficult to argue against this choice. There was an equally good suggestion --quiet we cannot refute.

I disagree that this is a viable workaround since nm produces quite a few more errors that you likely don't want to filter out

I think it is case by case. For this one, 2> /dev/null is a simple and easy workaround. I find it difficult to argue against this choice. There was an equally good suggestion --quiet we cannot refute.

I disagree that this is a viable workaround since nm produces quite a few more errors that you likely don't want to filter out

+1 to this - in fact, I already pointed this out further up in the comments.

I am strongly opposed to us having to get buy-in from GNU binutils to be able to add new options - I don't think has ever been agreed upon widely. Requiring this coordination stifles addition of new features in our toolchain. Over the years, we have added many options and features to the LLVM binutils that diverge from what GNU have done. As long as they don't clash, and have a valid use case, I don't think this is a bad thing - if GNU developers want to later implement it themselves, there's nothing stopping them, but I don't think we should be forced into coordination on it. The one thing I would say is that we should avoid using option names that have the potential to clash, for example single character aliases, unless there's some coordination.

I think it is case by case. For this one, 2> /dev/null is a simple and easy workaround. I find it difficult to argue against this choice. There was an equally good suggestion --quiet we cannot refute.

I disagree that this is a viable workaround since nm produces quite a few more errors that you likely don't want to filter out

+1 to this - in fact, I already pointed this out further up in the comments.

I am strongly opposed to us having to get buy-in from GNU binutils to be able to add new options - I don't think has ever been agreed upon widely. Requiring this coordination stifles addition of new features in our toolchain. Over the years, we have added many options and features to the LLVM binutils that diverge from what GNU have done. As long as they don't clash, and have a valid use case, I don't think this is a bad thing - if GNU developers want to later implement it themselves, there's nothing stopping them, but I don't think we should be forced into coordination on it. The one thing I would say is that we should avoid using option names that have the potential to clash, for example single character aliases, unless there's some coordination.

I have expressed my opinion clearly:

I think it is case by case. For this one, 2> /dev/null is a simple and easy workaround. I find it difficult to argue against this choice. There was an equally good suggestion --quiet we cannot refute.

In general I don't object to new LLVM specific options, but that should be useful and try avoiding potential clash with GNU. no symbols is an area where the GNU behavior may want to improve (you can dig up the history of our no symbols support - it is complex), so I believe this is an area where they'd like to have an opinion as well.
I don't see how --no-warning-for-no-symbols is better than --quiet, but --quiet is an option name they could use for other purposes.

You can also filter out no symbols and keep other warnings.

What, if any, is the community opinion on hidden flag use? Can this be a hidden flag and be done with it? I assume means hidden means no forward guarantees, rely on it at your own discretion.

I don't see how --no-warning-for-no-symbols is better than --quiet, but --quiet is an option name they could use for other purposes.

seems like you answer your own question :)

What, if any, is the community opinion on hidden flag use? Can this be a hidden flag and be done with it? I assume means hidden means no forward guarantees, rely on it at your own discretion.

My opinion: in this context, I don't think it buys us enough, but I'm not opposed to hidden flags in general. I don't know what the LLVM policy on hidden flags generally is though - more often than not, I've found hidden flags to be hidden accidentally!

I don't see how --no-warning-for-no-symbols is better than --quiet, but --quiet is an option name they could use for other purposes.

seems like you answer your own question :)

Exactly: if --quiet has the risk of being used for something else, we should avoid using it (just the same as for short single-letter aliases). If on the other hand, someone wants to coordinate with GNU and/or they happen to implement --quiet to cover this case, we can remove the --no-warning-for-no-symbols option at that point, and direct users towards --quiet instead. I don't think it should be a requirement to do this coordination, especially given how easy it would be to migrate in this case.

What, if any, is the community opinion on hidden flag use? Can this be a hidden flag and be done with it? I assume means hidden means no forward guarantees, rely on it at your own discretion.

My opinion: in this context, I don't think it buys us enough, but I'm not opposed to hidden flags in general. I don't know what the LLVM policy on hidden flags generally is though - more often than not, I've found hidden flags to be hidden accidentally!

I don't see how --no-warning-for-no-symbols is better than --quiet, but --quiet is an option name they could use for other purposes.

seems like you answer your own question :)

Exactly: if --quiet has the risk of being used for something else, we should avoid using it (just the same as for short single-letter aliases). If on the other hand, someone wants to coordinate with GNU and/or they happen to implement --quiet to cover this case, we can remove the --no-warning-for-no-symbols option at that point, and direct users towards --quiet instead. I don't think it should be a requirement to do this coordination, especially given how easy it would be to migrate in this case.

I don't agree. If the coordination is done first, we have a potential to use --quiet to right away. Then we don't need to have a temporary --no-warning-for-no-symbols which will eventually be removed (and potential to break users).
The no symbols has caused us lots of trouble (see the history). I imagined that binutils folks would have some opinions on it. Given the contentious previous discussions, I think it is entirely justified to ask them for an opinion.
I have said in some other reviews, their objection does not necessarily block us to add an option, as we could get a promise that they will not add an option to deliberately make us incompatible.

I don't agree. If the coordination is done first, we have a potential to use --quiet to right away. Then we don't need to have a temporary --no-warning-for-no-symbols which will eventually be removed (and potential to break users).

Whilst what you are saying is entirely true, the issue is that it unnecessarily (in my opinion) raises the bar on a contribution that is already small and quite simple. Not all developers know or care how to discuss things with the GNU community. Saying that they do makes it more likely that they will simply walk away from contributing what might otherwise be a useful patch. In this case, it's clearly not a code quality issue, nor is it a tricky thing for us to change in the future should some future developer decide they want to discuss the option with GNU/GNU independently go their own way.

The no symbols has caused us lots of trouble (see the history). I imagined that binutils folks would have some opinions on it. Given the contentious previous discussions, I think it is entirely justified to ask them for an opinion.

Could you point out where these contentious previous discussions were? I looked back through the history, and there didn't seem to be any real issues - it was just about getting the llvm-nm implementation right so that it matches the GNU behaviour, but it's quite possible I missed something, or I might just be misunderstanding you. I don't think difficulties trying to get our implementation to match GNU's are really relevant to this patch, since this is about a new option, so won't impact the default behaviour.

@jhenderson I'm glad you brought up the bar to contribution, because it crossed my mind too. It seems most features added to llvm tools did not need to coordinate with binutils.

nor is it a tricky thing for us to change in the future

👍

MaskRay added a comment.EditedFeb 26 2021, 9:36 AM

You can use --quiet instead. They accepted my --quiet patch: https://sourceware.org/bugzilla/show_bug.cgi?id=27408

I don't agree. If the coordination is done first, we have a potential to use --quiet to right away. Then we don't need to have a temporary --no-warning-for-no-symbols which will eventually be removed (and potential to break users).

Whilst what you are saying is entirely true, the issue is that it unnecessarily (in my opinion) raises the bar on a contribution that is already small and quite simple. Not all developers know or care how to discuss things with the GNU community. Saying that they do makes it more likely that they will simply walk away from contributing what might otherwise be a useful patch. In this case, it's clearly not a code quality issue, nor is it a tricky thing for us to change in the future should some future developer decide they want to discuss the option with GNU/GNU independently go their own way.

Discussing with binutils is case by case. This feature solves a minor insurmountable problem.
("no symbols" can be easily filtered out in the log.)
--quiet is a common option which folks may want to have an opinion on.
We would have migration cost that adds a different option first, adds --quiet for compatibility, then drops the first option.

The no symbols has caused us lots of trouble (see the history). I imagined that binutils folks would have some opinions on it. Given the contentious previous discussions, I think it is entirely justified to ask them for an opinion.

Could you point out where these contentious previous discussions were? I looked back through the history, and there didn't seem to be any real issues - it was just about getting the llvm-nm implementation right so that it matches the GNU behaviour, but it's quite possible I missed something, or I might just be misunderstanding you. I don't think difficulties trying to get our implementation to match GNU's are really relevant to this patch, since this is about a new option, so won't impact the default behaviour.

We added no symbols, then dropped the diagnostic if there is hidden symbols (e.g. ARM mapping symbols $a which are normally not display for arm-*-nm), then changed the output stream from stdout to stderr.

keith reclaimed this revision.Feb 27 2021, 5:54 PM
This revision now requires changes to proceed.Feb 27 2021, 5:54 PM
keith updated this revision to Diff 326941.Feb 27 2021, 5:54 PM

Update to --quiet spelling

keith added a comment.Feb 27 2021, 5:54 PM

Thanks for driving that! Updated my patch to use --quiet. I did hesitate a bit on specifically saying 'no symbols' in the help string, but I guess we can update that in the future if anything else makes sense to gate with this flag as well.

keith edited the summary of this revision. (Show Details)Feb 27 2021, 5:54 PM
keith updated this revision to Diff 326942.Feb 27 2021, 6:35 PM
keith edited the summary of this revision. (Show Details)

Add flag to test

You need to add the --quiet option to the llvm-nm documentation page, which you can find at llvm/docs/CommandGuide/llvm-nm.rst.

llvm/test/tools/llvm-nm/X86/nm-no-symbols.test
13

I should have noticed this before. Could you replace the FileCheck line with count 0 instead? That'll be simpler, I think.

llvm/tools/llvm-nm/llvm-nm.cpp
161–162

I don't think these options are in any particular order, but it would probably make sense not to split up the two size-related options! I think you can just move this somewhere else.

keith updated this revision to Diff 328622.Mar 5 2021, 12:54 PM
keith marked 2 inline comments as done.

Update test and docs

MaskRay accepted this revision.Mar 6 2021, 10:35 AM
MaskRay added inline comments.
llvm/docs/CommandGuide/llvm-nm.rst
209
This revision is now accepted and ready to land.Mar 6 2021, 10:35 AM
keith updated this revision to Diff 328913.Mar 7 2021, 4:09 PM
keith marked an inline comment as done.

Update help

This revision was landed with ongoing or failed builds.Mar 7 2021, 4:20 PM
This revision was automatically updated to reflect the committed changes.
keith added a comment.Mar 7 2021, 4:21 PM

Thanks again everyone!