diff mbox series

gas: accept custom ".linefile <n> ."

Message ID 40e89395-1438-6cbe-aa37-1a04a724c8c7@suse.com
State Accepted
Headers
Series gas: accept custom ".linefile <n> ." |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Jan Beulich Nov. 11, 2022, 7:32 a.m. UTC 
  While .linefile is generally intended for gas internal use only, its use
in a source file would better not result in an internal error. Give use
of it outside of any macro(-like) construct the meaning of restoring the
original (physical) input file name.

Comments

Nick Clifton Nov. 14, 2022, 1:43 p.m. UTC | #1 
Hi Jan,

> While .linefile is generally intended for gas internal use only, its use
> in a source file would better not result in an internal error.

Agreed.

> Give use
> of it outside of any macro(-like) construct the meaning of restoring the
> original (physical) input file name.

We should document this behaviour in the as.texi file then, so that it
is "official"...

> --- a/gas/input-scrub.c
> +++ b/gas/input-scrub.c
> @@ -469,13 +469,15 @@ new_logical_line_flags (const char *fnam
>         /* FIXME: we could check that include nesting is correct.  */
>         break;
>       case 1 << 3:
> -      if (line_number < 0 || fname != NULL || next_saved_file == NULL)
> +      if (line_number < 0 || fname != NULL)
>   	abort ();

I still hate having calls to abort() in code.  Much better to have
a helpful error message IMHO.  Not important for this patch though.

Cheers
   Nick
Jan Beulich Nov. 14, 2022, 3:53 p.m. UTC | #2 
On 14.11.2022 14:43, Nick Clifton wrote:
>> While .linefile is generally intended for gas internal use only, its use
>> in a source file would better not result in an internal error.
> 
> Agreed.
> 
>> Give use
>> of it outside of any macro(-like) construct the meaning of restoring the
>> original (physical) input file name.
> 
> We should document this behaviour in the as.texi file then, so that it
> is "official"...

Do you really think we should? .linefile as a whole is undocumented. If
people use it, then at their own risk (if we made things "official",
later we couldn't later behavior anymore). The point of the change here
was merely to avoid an internal error that I had noticed. The particular
behavior chosen was simply the one easiest to implement (and at least
halfway sensible) while addressing the issue.

I am, btw, doing further work in this area, which includes zapping
.linefile from listings (where I think it should never have appeared,
but I didn't think of that aspect several months ago). Such zapping
would imo also be unreasonable if the directive became "official".

Jan
Nick Clifton Nov. 14, 2022, 4:02 p.m. UTC | #3 
Hi Jan,

>> We should document this behaviour in the as.texi file then, so that it
>> is "official"...
> 
> Do you really think we should? .linefile as a whole is undocumented. If
> people use it, then at their own risk (if we made things "official",
> later we couldn't later behavior anymore).

Alright - in which case we should document it as being unsupported/not
for use.

I am just trying to avoid the situation where a future binutils user
discovers the directive, tries it out and then complains about it not
working as expected/documented/whatever.

Cheers
   Nick
Jan Beulich Nov. 14, 2022, 4:16 p.m. UTC | #4 
On 14.11.2022 17:02, Nick Clifton wrote:
>>> We should document this behaviour in the as.texi file then, so that it
>>> is "official"...
>>
>> Do you really think we should? .linefile as a whole is undocumented. If
>> people use it, then at their own risk (if we made things "official",
>> later we couldn't later behavior anymore).
> 
> Alright - in which case we should document it as being unsupported/not
> for use.
> 
> I am just trying to avoid the situation where a future binutils user
> discovers the directive, tries it out and then complains about it not
> working as expected/documented/whatever.

Okay, if you could give me a hint where undocumented stuff is to go
in as.texi, I could see about making a patch. Otherwise if you would
make one, it could still educate me in case something else needs
similar treatment going forward.

Jan
Jan Beulich Nov. 15, 2022, 7:59 a.m. UTC | #5 
On 14.11.2022 14:43, Nick Clifton wrote:
>> --- a/gas/input-scrub.c
>> +++ b/gas/input-scrub.c
>> @@ -469,13 +469,15 @@ new_logical_line_flags (const char *fnam
>>         /* FIXME: we could check that include nesting is correct.  */
>>         break;
>>       case 1 << 3:
>> -      if (line_number < 0 || fname != NULL || next_saved_file == NULL)
>> +      if (line_number < 0 || fname != NULL)
>>   	abort ();
> 
> I still hate having calls to abort() in code.  Much better to have
> a helpful error message IMHO.  Not important for this patch though.

So do you consider gas_assert() better? I've been using abort() (here
and elsewhere) only because of pre-existing adjacent uses and H.J. at
some point (in x86 code) asking me to use that in favor of gas_assert().
Personally I'd prefer making things like the above assertions. But
perhaps such could also be as_fatal().

Jan
Nick Clifton Nov. 15, 2022, 11:16 a.m. UTC | #6 
Hi Jan,

>> Alright - in which case we should document it as being unsupported/not
>> for use.

> Okay, if you could give me a hint where undocumented stuff is to go
> in as.texi, I could see about making a patch.

There isn't one. :-)

There is however a node called "AS Sections" which details the assembler
specific sections like .expr.  So there is a (very small) precedence for
this kind of thing.

> Otherwise if you would
> make one, it could still educate me in case something else needs
> similar treatment going forward.

OK, I will look into it.  In the meantime please consider my suggestion
withdrawn.

Cheers
   Nick
Nick Clifton Nov. 15, 2022, 11:23 a.m. UTC | #7 
Hi Jan,

>>>    	abort ();
>>
>> I still hate having calls to abort() in code.  Much better to have
>> a helpful error message IMHO.  Not important for this patch though.
> 
> So do you consider gas_assert() better? 

Not really.  It still does not tell the user what has gone wrong, nor
does it help them avoid the problem.  Sure they can report it, and having
an abort makes it easier for an investigator to use a debugger to catch
the problem, but in the meantime the user is still left wondering what
went wrong.

The assembler (and other tools) already have a well understood way of
telling the user that something has gone wrong - the exit status code,
along with, hopefully, an error message.  So I think that it is best
to make use of it.

 > But perhaps such could also be as_fatal().

Given a choice, I would definitely prefer as_fatal() over gas_assert()
or abort().  But this is just my preference.  I am not laying down the
law here, and I am not going to reject patches just because they contain
calls to abort.

Cheers
   Nick
diff mbox series

Patch

--- a/gas/input-scrub.c
+++ b/gas/input-scrub.c
@@ -469,13 +469,15 @@  new_logical_line_flags (const char *fnam
       /* FIXME: we could check that include nesting is correct.  */
       break;
     case 1 << 3:
-      if (line_number < 0 || fname != NULL || next_saved_file == NULL)
+      if (line_number < 0 || fname != NULL)
 	abort ();
       /* PR gas/16908 workaround: Ignore updates when nested inside a macro
 	 expansion.  */
       if (from_sb_expansion == expanding_nested)
 	return;
-      if (next_saved_file->logical_input_file)
+      if (next_saved_file == NULL)
+	fname = physical_input_file;
+      else if (next_saved_file->logical_input_file)
 	fname = next_saved_file->logical_input_file;
       else
 	fname = next_saved_file->physical_input_file;